Skip to content

Refactor CD scripts to eliminate details.txt dependency and support m… - #4972

Draft
anushka567 wants to merge 2 commits into
masterfrom
anushkadhn/refactors-for-nightly-louhi
Draft

Refactor CD scripts to eliminate details.txt dependency and support m…#4972
anushka567 wants to merge 2 commits into
masterfrom
anushkadhn/refactors-for-nightly-louhi

Conversation

@anushka567

Copy link
Copy Markdown
Member

…ulti-project Louhi pipelines

Please ensure your PR title follows the format:

type(scope): subject

Example:
feat(api): add user login endpoint

Available types:

  • feat: A new feature
  • fix: A bug fix
  • docs: Documentation only changes
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
  • refactor: A code change that neither fixes a bug nor adds a feature
  • perf: A code change that improves performance
  • test: Adding missing tests or correcting existing tests
  • build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
  • ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
  • chore: Other changes that don't modify src or test files
  • revert: Reverts a previous commit

Description

Link to the issue in case of a bug fix.

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - NA

Any backward incompatible change? If so, please explain.

@github-actions

Copy link
Copy Markdown

Hey there and thank you for opening this pull request! 👋🏼

We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted.

Details:

No release type found in pull request title "Refactor CD scripts to eliminate details.txt dependency and support m…". Add a prefix to indicate what kind of release this pull request corresponds to. For reference, see https://www.conventionalcommits.org/

Available types:
 - feat: A new feature
 - fix: A bug fix
 - docs: Documentation only changes
 - style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc)
 - refactor: A code change that neither fixes a bug nor adds a feature
 - perf: A code change that improves performance
 - test: Adding missing tests or correcting existing tests
 - build: Changes that affect the build system or external dependencies (example scopes: gulp, broccoli, npm)
 - ci: Changes to our CI configuration files and scripts (example scopes: Travis, Circle, BrowserStack, SauceLabs)
 - chore: Other changes that don't modify src or test files
 - revert: Reverts a previous commit

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the end-to-end and installation test scripts to fetch configuration parameters (such as release version, commit hash, and upload bucket) directly from VM metadata attributes or environment variables, falling back to GCS if unavailable. It also improves OS distribution detection and version verification. The review feedback highlights three key areas for improvement: explicitly handling GCS copy failures in e2e_test.sh to avoid silent errors, adhering to the Google Shell Style Guide by removing the redundant function keyword syntax in install_test.sh, and validating that the release version is not empty before proceeding with package installation.

Comment on lines +113 to +115
# Fallback to fetching details.txt from GCS if metadata is not provided
gcloud storage cp gs://${BUCKET_NAME_TO_USE}/version-detail/details.txt . || true
echo "$VM_NAME" >> details.txt

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If the gcloud storage cp command fails, details.txt will not be created. The subsequent echo "$VM_NAME" >> details.txt will then create a new file containing only the VM name. This causes the script to later parse the VM name as the VERSION (line 236) and leave COMMIT_HASH empty, leading to silent failures. We should handle the copy failure explicitly using an explicit if/else structure.

Suggested change
# Fallback to fetching details.txt from GCS if metadata is not provided
gcloud storage cp gs://${BUCKET_NAME_TO_USE}/version-detail/details.txt . || true
echo "$VM_NAME" >> details.txt
# Fallback to fetching details.txt from GCS if metadata is not provided
if gcloud storage cp gs://${BUCKET_NAME_TO_USE}/version-detail/details.txt .; then
echo "$VM_NAME" >> details.txt
else
echo "Error: Failed to fetch details.txt from GCS fallback." >&2
exit 1
fi
References
  1. In shell scripts, use an explicit if/else structure for fallback logic instead of the && || chain to improve readability and avoid unexpected behavior.

to_release_version=$(sed '1q' details.txt | tr -d '\n')
echo $vm_instance_name >> details.txt
# Helper function to fetch metadata value from GCE metadata server
function fetch_meta_data_value() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

According to the Google Shell Style Guide, we should not combine the function keyword with () when defining functions. Use either func_name() { ... } or function func_name { ... }, with the former being preferred.

Suggested change
function fetch_meta_data_value() {
fetch_meta_data_value() {
References
  1. According to the Google Shell Style Guide, we should not combine the function keyword with () when defining functions. (link)

Comment on lines +83 to +88
# Backward compatibility fallback if RELEASE_VERSION is not present in metadata
if [[ -z "$to_release_version" ]]; then
if gcloud storage cp "gs://${upload_bucket}/version-detail/details.txt" ./details.txt 2>/dev/null; then
to_release_version=$(sed -n 1p details.txt | tr -d '\r\n')
fi
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If to_release_version cannot be retrieved from either the metadata server or the fallback details.txt file, it remains empty. This will cause subsequent package installation commands (like apt-get install or yum install) to fail with syntax/package errors. We should validate that to_release_version is not empty and exit early with a clear error message using an explicit if/else structure.

Suggested change
# Backward compatibility fallback if RELEASE_VERSION is not present in metadata
if [[ -z "$to_release_version" ]]; then
if gcloud storage cp "gs://${upload_bucket}/version-detail/details.txt" ./details.txt 2>/dev/null; then
to_release_version=$(sed -n 1p details.txt | tr -d '\r\n')
fi
fi
# Backward compatibility fallback if RELEASE_VERSION is not present in metadata
if [[ -z "$to_release_version" ]]; then
if gcloud storage cp "gs://${upload_bucket}/version-detail/details.txt" ./details.txt 2>/dev/null; then
to_release_version=$(sed -n 1p details.txt | tr -d '\r\n')
fi
fi
if [[ -z "$to_release_version" ]]; then
echo "Error: RELEASE_VERSION is empty and could not be retrieved." >&2
exit 1
fi
References
  1. In shell scripts, use an explicit if/else structure for fallback logic instead of the && || chain to improve readability and avoid unexpected behavior.

@anushka567
anushka567 force-pushed the anushkadhn/refactors-for-nightly-louhi branch from 3138701 to f3557a8 Compare August 4, 2026 07:59
@anushka567
anushka567 force-pushed the anushkadhn/refactors-for-nightly-louhi branch from f3557a8 to e735085 Compare August 4, 2026 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant