CI: remove light-aws-builder specs, use binary from image.#658
Open
aramprice wants to merge 1 commit into
Open
CI: remove light-aws-builder specs, use binary from image.#658aramprice wants to merge 1 commit into
aramprice wants to merge 1 commit into
Conversation
Contributor
WalkthroughThe pull request removes four AWS CI jobs (test-aws-unit, test-aws-drivers, test-aws-integration, cleanup-light-aws-builder-test-amis) and their task definitions from the Concourse pipeline. The build_light_aws_stemcell_new YTT helper is refactored to accept prefix-based parameters (prefix, stemcell_os, stemcell_version, efi, ami_destinations, ami_excluded_destinations) and the build-light-aws job plan is reworked to use the new signature with aws-light-stemcell-builder-registry-image. The light-aws-builder-src resource is removed and the registry-image resource is renamed from light-stemcell-builder-registry-image to aws-light-stemcell-builder-registry-image. The Google make-raw-from-heavy-stemcell task is updated to use gce-cpi-release-registry-image. The light-stemcell-builder binary is invoked directly instead of go run main.go. Builder-src inputs are removed from build.yml, cleanup-ami.yml, run-upload-test.yml, and us-gov-merge-builds.yml. Bash parameter validation is hardened throughout by replacing unquoted Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
aramprice
requested review from
a team and
lnguyen
and removed request for
a team
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/pipelines/publisher.yml`:
- Line 19: The image reference `light-stemcell-builder-registry-image` on line
19 does not match the actual resource name that has been renamed to
`aws-light-stemcell-builder-registry-image` (as defined at line 751 and fetched
by the job at line 130). Update the image field on line 19 from
`light-stemcell-builder-registry-image` to
`aws-light-stemcell-builder-registry-image` to resolve the artifact name
mismatch so the build-<prefix>-stemcell task can locate the correct image at
runtime.
In `@ci/tasks/light-google/create-public-image.sh`:
- Around line 65-67: The `touch image` command on line 65 only updates the
modification time without clearing existing file contents, which means if the
image file already exists from a previous extraction, it will retain the full
heavy payload when calculating the SHA1 hash and packaging into the tar archive.
Replace `touch image` with a command that actually empties the file contents,
such as using output redirection (> image) or truncate, to ensure image is
created as an empty light placeholder before the SHA1 calculation on line 66.
- Around line 53-54: The guest_os_features_flag variable is being passed as a
quoted argument to gcloud compute images create even when it is empty, which
causes gcloud argument parsing to fail for configurations with no features
enabled. Remove the quotes around the guest_os_features_flag variable and
conditionally include the flag only when it contains a non-empty value, such as
by using parameter expansion (e.g., only adding the flag and its value when the
variable is not empty).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaf392a4-d67f-4fbf-8849-6245785807f6
📒 Files selected for processing (13)
ci/pipelines/publisher.ymlci/tasks/light-aws/build.shci/tasks/light-aws/cleanup-ami.shci/tasks/light-aws/cleanup-ami.ymlci/tasks/light-aws/test-drivers.shci/tasks/light-aws/test-drivers.ymlci/tasks/light-aws/test-integration.shci/tasks/light-aws/test-integration.ymlci/tasks/light-aws/test-unit.shci/tasks/light-aws/test-unit.ymlci/tasks/light-google/create-public-image.shci/tasks/light-google/deploy-skeletal.shci/tasks/light-google/make-raw-from-heavy-stemcell.sh
💤 Files with no reviewable changes (8)
- ci/tasks/light-aws/test-integration.yml
- ci/tasks/light-aws/cleanup-ami.yml
- ci/tasks/light-aws/cleanup-ami.sh
- ci/tasks/light-aws/test-drivers.yml
- ci/tasks/light-aws/test-unit.sh
- ci/tasks/light-aws/test-unit.yml
- ci/tasks/light-aws/test-drivers.sh
- ci/tasks/light-aws/test-integration.sh
github-project-automation
Bot
moved this from Inbox
to Waiting for Changes | Open for Contribution
in Foundational Infrastructure Working Group
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
aramprice
force-pushed
the
remove-aws-light-stemcell-tests
branch
from
5ee4a96 to
0f82ba6
Compare
aramprice
force-pushed
the
remove-aws-light-stemcell-tests
branch
from
0f82ba6 to
f80d485
Compare
github-project-automation
Bot
moved this from Waiting for Changes | Open for Contribution
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
These specs moved to that pipeline, and the docker image now contains the `light-aws-builder` binary. Also contains: - shellcheck cleanup - removes aws-light-stemcell-builder src
aramprice
force-pushed
the
remove-aws-light-stemcell-tests
branch
from
f80d485 to
3fcb013
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/tasks/light-aws/cleanup-ami.sh`:
- Around line 14-16: The parameter expansion guards for AWS credentials in the
export statements for AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and
AWS_DEFAULT_REGION are using the `?` operator which only checks if variables are
set, but allows empty values. Change each guard from the current
`${variable?'must be set'}` format to `${variable:?'must be set'}` format by
adding a colon before the question mark. This will ensure that the script fails
immediately if any of these credential variables are empty or unset, rather than
passing validation and failing later during AWS API calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8e0214ee-af7c-4d1a-bc21-f711bf6d1f9d
📒 Files selected for processing (17)
ci/pipelines/publisher.ymlci/tasks/light-aws/build.shci/tasks/light-aws/build.ymlci/tasks/light-aws/cleanup-ami.shci/tasks/light-aws/cleanup-ami.ymlci/tasks/light-aws/run-upload-test.shci/tasks/light-aws/run-upload-test.ymlci/tasks/light-aws/test-drivers.shci/tasks/light-aws/test-drivers.ymlci/tasks/light-aws/test-integration.shci/tasks/light-aws/test-integration.ymlci/tasks/light-aws/test-unit.shci/tasks/light-aws/test-unit.ymlci/tasks/light-aws/us-gov-merge-builds.ymlci/tasks/light-google/create-public-image.shci/tasks/light-google/deploy-skeletal.shci/tasks/light-google/make-raw-from-heavy-stemcell.sh
💤 Files with no reviewable changes (8)
- ci/tasks/light-aws/test-drivers.yml
- ci/tasks/light-aws/test-unit.yml
- ci/tasks/light-aws/test-unit.sh
- ci/tasks/light-aws/build.yml
- ci/tasks/light-aws/test-integration.sh
- ci/tasks/light-aws/us-gov-merge-builds.yml
- ci/tasks/light-aws/test-integration.yml
- ci/tasks/light-aws/test-drivers.sh
Comment on lines
+14
to
+16
| export AWS_ACCESS_KEY_ID=${ami_access_key?'must be set'} | ||
| export AWS_SECRET_ACCESS_KEY=${ami_secret_key?'must be set'} | ||
| export AWS_DEFAULT_REGION=${ami_region?'must be set'} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -eu -o pipefail
ami_access_key=""
ami_secret_key=""
ami_region="us-east-1"
echo "Current behavior (\${var?}): empty values are accepted"
AWS_ACCESS_KEY_ID=${ami_access_key?'must be set'}
AWS_SECRET_ACCESS_KEY=${ami_secret_key?'must be set'}
echo "accepted: '${AWS_ACCESS_KEY_ID}' / '${AWS_SECRET_ACCESS_KEY}'"
echo "Expected strict behavior (\${var:?}): empty values are rejected"
if : "${ami_access_key:?must be set}"; then
echo "unexpected: strict check passed"
fiRepository: cloudfoundry/bosh-linux-stemcell-builder
Length of output: 285
Use :? guards so empty AWS credentials fail fast.
Lines 14-16 currently use ${var?'must be set'}, which permits empty-but-set values. With this task's default params (ci/tasks/light-aws/cleanup-ami.yml), empty credentials pass these checks and fail later in AWS calls instead of failing immediately.
Suggested fix
- export AWS_ACCESS_KEY_ID=${ami_access_key?'must be set'}
- export AWS_SECRET_ACCESS_KEY=${ami_secret_key?'must be set'}
- export AWS_DEFAULT_REGION=${ami_region?'must be set'}
+ : "${ami_access_key:?must be set}"
+ : "${ami_secret_key:?must be set}"
+ : "${ami_region:?must be set}"
+ export AWS_ACCESS_KEY_ID="${ami_access_key}"
+ export AWS_SECRET_ACCESS_KEY="${ami_secret_key}"
+ export AWS_DEFAULT_REGION="${ami_region}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export AWS_ACCESS_KEY_ID=${ami_access_key?'must be set'} | |
| export AWS_SECRET_ACCESS_KEY=${ami_secret_key?'must be set'} | |
| export AWS_DEFAULT_REGION=${ami_region?'must be set'} | |
| : "${ami_access_key:?must be set}" | |
| : "${ami_secret_key:?must be set}" | |
| : "${ami_region:?must be set}" | |
| export AWS_ACCESS_KEY_ID="${ami_access_key}" | |
| export AWS_SECRET_ACCESS_KEY="${ami_secret_key}" | |
| export AWS_DEFAULT_REGION="${ami_region}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ci/tasks/light-aws/cleanup-ami.sh` around lines 14 - 16, The parameter
expansion guards for AWS credentials in the export statements for
AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_DEFAULT_REGION are using the
`?` operator which only checks if variables are set, but allows empty values.
Change each guard from the current `${variable?'must be set'}` format to
`${variable:?'must be set'}` format by adding a colon before the question mark.
This will ensure that the script fails immediately if any of these credential
variables are empty or unset, rather than passing validation and failing later
during AWS API calls.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
These specs moved to that pipeline, and the docker image now contains the
light-aws-builderbinary.NOTE: the
publisherpipeline must be reconfigured after merging.