-
Notifications
You must be signed in to change notification settings - Fork 166
toil: add bump-apollo-ci references tool #17565
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at b95e848. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17565 +/- ##
==========================================
- Coverage 48.72% 48.72% -0.01%
==========================================
Files 2728 2728
Lines 203127 203127
==========================================
- Hits 98980 98976 -4
- Misses 96399 96403 +4
Partials 7748 7748
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey there - I've reviewed your changes - here's some feedback:
- Please update the PR description and remove the placeholder 'change me!' sections so reviewers understand the intent of your tool and confirm any required documentation updates.
- Add a preliminary check in the bump script to ensure the working directory is clean (no uncommitted changes), so you don’t accidentally overwrite in-flight work.
- Consider validating that required dependencies (e.g., the GitHub CLI
gh) are installed and configured before attempting to create or update PRs for a smoother developer experience.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Please update the PR description and remove the placeholder 'change me!' sections so reviewers understand the intent of your tool and confirm any required documentation updates.
- Add a preliminary check in the bump script to ensure the working directory is clean (no uncommitted changes), so you don’t accidentally overwrite in-flight work.
- Consider validating that required dependencies (e.g., the GitHub CLI `gh`) are installed and configured before attempting to create or update PRs for a smoother developer experience.
## Individual Comments
### Comment 1
<location> `tools/bump-apollo-ci/bump.sh:41` </location>
<code_context>
+ local version="$1"
+ local files
+
+ files=$(git grep -l "quay\.io/stackrox-io/apollo-ci:" -- . ':!tools/bump-apollo-ci/' || true)
+
+ while IFS= read -r file; do
</code_context>
<issue_to_address>
**issue (bug_risk):** The exclusion pattern may not work as intended with git grep.
The ':!tools/bump-apollo-ci/' exclusion may not be reliable across git versions. Use 'find' or post-process the results in bash for consistent exclusion.
</issue_to_address>
### Comment 2
<location> `tools/bump-apollo-ci/bump.sh:136-139` </location>
<code_context>
+
+ git push -u origin "$current_branch"
+
+ local existing_pr
+ existing_pr=$(gh pr list --head "$current_branch" --json number,url --jq '.[0] | "\(.number)|\(.url)"' 2>/dev/null || true)
+
+ if [[ -n "$existing_pr" ]] && [[ "$existing_pr" != "null|null" ]] && [[ "$existing_pr" != "|" ]]; then
+ local pr_number="${existing_pr%%|*}"
+ local pr_url="${existing_pr#*|}"
</code_context>
<issue_to_address>
**suggestion:** Parsing PR info with string splitting is fragile.
Using '|' as a delimiter is unreliable if the PR title or URL contains this character. Switching to JSON parsing will ensure accurate extraction of PR details.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| local existing_pr | ||
| existing_pr=$(gh pr list --head "$current_branch" --json number,url --jq '.[0] | "\(.number)|\(.url)"' 2>/dev/null || true) | ||
|
|
||
| if [[ -n "$existing_pr" ]] && [[ "$existing_pr" != "null|null" ]] && [[ "$existing_pr" != "|" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Parsing PR info with string splitting is fragile.
Using '|' as a delimiter is unreliable if the PR title or URL contains this character. Switching to JSON parsing will ensure accurate extraction of PR details.
Description
change me!
User-facing documentation
Testing and quality
Automated testing
How I validated my change
change me!