docs(release): fix release procedure — 5 functions, missing params, DefaultAMI bump#85
Merged
Merged
Conversation
…DefaultInstanceType params and DefaultAMI bump step The live jit-runners stack has 5 Lambda functions (rebalancer added) and 19 parameters; the documented update-stack omitted RebalancerLambdaS3Key, RunnerVersion, and DefaultInstanceType, so it would fail on the current stack. Also documents bumping DefaultAMI to the tag-built AMI (new step 2b + step 5). Signed-off-by: Kaio Fellipe <kaio6fellipe@gmail.com>
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughUpdated release procedure documentation in ChangesRelease Procedure Documentation Update
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
OpenSSF Scorecard — 8.3/10 ✅
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/release.md`:
- Around line 62-63: The docs are inconsistent: step 2b instructs to set
DefaultAMI to UsePreviousValue when not rolling the runner image, but step 5
hardcodes ${NEW_AMI}, which breaks the non-rolling path; update the text in step
5 (and the similar occurrence around line 107) to conditionally show the
CloudFormation/CLI snippet that uses DefaultAMI: UsePreviousValue when NEW_AMI
is unset or to ${NEW_AMI} when provided, and add a short note showing the exact
replacement (i.e., show both variants or an if/otherwise sentence) so references
to DefaultAMI, UsePreviousValue, and ${NEW_AMI} are consistent across the
document.
- Around line 56-60: After capturing NEW_AMI from the AWS query (the aws ec2
describe-images invocation that sets NEW_AMI), add a fail-fast guard that checks
if NEW_AMI is empty or "None" and, if so, prints a clear error to stderr and
exits non-zero so you don't proceed to Step 5 with an invalid DefaultAMI; ensure
the message references the original query and suggests retrying or checking
filters to aid troubleshooting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+56
to
+60
| NEW_AMI=$(aws ec2 describe-images --owners self --region us-east-2 \ | ||
| --filters "Name=name,Values=jit-runner-vX.Y.Z*" "Name=is-public,Values=true" \ | ||
| --query 'reverse(sort_by(Images,&CreationDate))[0].ImageId' --output text) | ||
| echo "New AMI: ${NEW_AMI}" | ||
| ``` |
There was a problem hiding this comment.
Add a fail-fast guard after AMI lookup
If the AMI query returns None/empty (build delay, filter miss), Step 5 will pass an invalid DefaultAMI value. Add a guard immediately after capture.
Suggested doc fix
NEW_AMI=$(aws ec2 describe-images --owners self --region us-east-2 \
--filters "Name=name,Values=jit-runner-vX.Y.Z*" "Name=is-public,Values=true" \
--query 'reverse(sort_by(Images,&CreationDate))[0].ImageId' --output text)
+if [ -z "${NEW_AMI}" ] || [ "${NEW_AMI}" = "None" ]; then
+ echo "Failed to resolve NEW_AMI for vX.Y.Z in us-east-2" >&2
+ exit 1
+fi
echo "New AMI: ${NEW_AMI}"📝 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
| NEW_AMI=$(aws ec2 describe-images --owners self --region us-east-2 \ | |
| --filters "Name=name,Values=jit-runner-vX.Y.Z*" "Name=is-public,Values=true" \ | |
| --query 'reverse(sort_by(Images,&CreationDate))[0].ImageId' --output text) | |
| echo "New AMI: ${NEW_AMI}" | |
| ``` | |
| NEW_AMI=$(aws ec2 describe-images --owners self --region us-east-2 \ | |
| --filters "Name=name,Values=jit-runner-vX.Y.Z*" "Name=is-public,Values=true" \ | |
| --query 'reverse(sort_by(Images,&CreationDate))[0].ImageId' --output text) | |
| if [ -z "${NEW_AMI}" ] || [ "${NEW_AMI}" = "None" ]; then | |
| echo "Failed to resolve NEW_AMI for vX.Y.Z in us-east-2" >&2 | |
| exit 1 | |
| fi | |
| echo "New AMI: ${NEW_AMI}" |
🤖 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 `@docs/release.md` around lines 56 - 60, After capturing NEW_AMI from the AWS
query (the aws ec2 describe-images invocation that sets NEW_AMI), add a
fail-fast guard that checks if NEW_AMI is empty or "None" and, if so, prints a
clear error to stderr and exits non-zero so you don't proceed to Step 5 with an
invalid DefaultAMI; ensure the message references the original query and
suggests retrying or checking filters to aid troubleshooting.
Comment on lines
+62
to
+63
| If you are not rolling the runner image this release, skip this and leave | ||
| `DefaultAMI` as `UsePreviousValue` in step 5. |
There was a problem hiding this comment.
Inconsistent DefaultAMI instructions vs command block
Step 2b says to use UsePreviousValue when not rolling the image, but Step 5 hardcodes ${NEW_AMI}. This can break the documented path when NEW_AMI is unset.
Suggested doc fix
- ParameterKey=DefaultAMI,ParameterValue=${NEW_AMI} \
+ # If rolling AMI:
+ # ParameterKey=DefaultAMI,ParameterValue=${NEW_AMI} \
+ # If NOT rolling AMI:
+ ParameterKey=DefaultAMI,UsePreviousValue=true \Also applies to: 107-107
🤖 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 `@docs/release.md` around lines 62 - 63, The docs are inconsistent: step 2b
instructs to set DefaultAMI to UsePreviousValue when not rolling the runner
image, but step 5 hardcodes ${NEW_AMI}, which breaks the non-rolling path;
update the text in step 5 (and the similar occurrence around line 107) to
conditionally show the CloudFormation/CLI snippet that uses DefaultAMI:
UsePreviousValue when NEW_AMI is unset or to ${NEW_AMI} when provided, and add a
short note showing the exact replacement (i.e., show both variants or an
if/otherwise sentence) so references to DefaultAMI, UsePreviousValue, and
${NEW_AMI} are consistent across the document.
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
1 participant
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.
Why
While running the v1.0.0-rc.5 release + prod rollout,
docs/release.mdwas found to be out of date with the livejit-runnersCloudFormation stack:rebalancer(5 Lambdas, 5 S3 keys). The doc documented onlywebhook/scaleup/scaledown/lifecycle.update-stackomittedRebalancerLambdaS3Key,RunnerVersion, andDefaultInstanceType. With--use-previous-template, CloudFormation requires every parameter, so the documented command would fail on the current 19-parameter stack.DefaultAMIbump — the AMI build (ami-build.yml) auto-runs on the release tag, but the procedure never captured the new AMI or rolled it into the stack.What changed
jit-runners-rebalancer.rebalancer.zip.NEW_AMI) fromus-east-2.update-stacknow lists all 19 parameters: addsRebalancerLambdaS3Key,RunnerVersion,DefaultInstanceType; setsDefaultAMI=${NEW_AMI}(with a note to useUsePreviousValueif not rolling the image);MaxReEnqueueAttempts→UsePreviousValue(was a one-time=3).DefaultAMI).vPREVreference tov1.0.0-rc.4.Verified against the live stack during the rc.5 rollout (the corrected 19-param command is exactly what succeeded).
🤖 Generated with Claude Code
Summary by CodeRabbit