-
Notifications
You must be signed in to change notification settings - Fork 332
update update_all_versions command #3219
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
Conversation
Signed-off-by: Kevin Su <[email protected]>
Code Review Agent Run #dd580dActionable Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3219 +/- ##
===========================================
- Coverage 77.35% 46.66% -30.70%
===========================================
Files 214 214
Lines 22363 22291 -72
Branches 2924 2923 -1
===========================================
- Hits 17300 10401 -6899
- Misses 4207 11362 +7155
+ Partials 856 528 -328 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wild-endeavor
left a comment
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.
but check first?
plugins/Makefile
Outdated
| # it exits with exit code 1 and github actions aborts the build. | ||
| ./run_all_plugins.sh grep "$(PLACEHOLDER)" "$(VERSION_FILE)" | ||
| ./run_all_plugins.sh sed -i "s/$(PLACEHOLDER)/__version__ = \"${VERSION}\"/g" $(VERSION_FILE) | ||
| ./run_all_plugins.sh sed -i "s/$(PLACEHOLDER)\/__version__ = \"${VERSION}\"/g" $(VERSION_FILE) |
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.
The sed command has an incorrect escape sequence. The backslash before the forward slash creates an incorrect pattern that won't match the placeholder correctly. Remove the backslash before the forward slash.
Code suggestion
Check the AI-generated fix before applying
| ./run_all_plugins.sh sed -i "s/$(PLACEHOLDER)\/__version__ = \"${VERSION}\"/g" $(VERSION_FILE) | |
| ./run_all_plugins.sh sed -i "s/$(PLACEHOLDER)/__version__ = \"${VERSION}\"/g" $(VERSION_FILE) |
Code Review Run #dd580d
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Code Review Agent Run #dc8e16Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
This reverts commit fe5f551.
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Code Review Agent Run #6d8558Actionable Suggestions - 1
Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help |
|
merge when ready. still highly confused as to why this happened. cc @eapolinario. comparing the last known good run and the first bad run, all the gh runner/image/etc versions were identical. sed shouldn't just change behavior. |
|
Comparing the two runs we can see that the issue is that the tag coming from https://github.com/flyteorg/flytekit/blob/master/.github/workflows/pythonpublish.yml#L39-L40 is different now:
It also repros locally: |
Signed-off-by: Kevin Su <[email protected]> Signed-off-by: Atharva <[email protected]>
Tracking issue
Why are the changes needed?
CI is failing https://github.com/flyteorg/flytekit/actions/runs/14344424181/job/40211181071
What changes were proposed in this pull request?
How was this patch tested?
tested it here https://github.com/flyteorg/flytekit/actions/runs/14349769057/job/40226120233?pr=3219
https://pypi.org/project/flytekitplugins-spark/#history
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This PR fixes the update_all_versions command in the plugins Makefile by adjusting variable assignment formatting, correcting version string definition and usage, and refactoring grep/sed commands within a shell context. These changes resolve CI failures caused by incorrect command syntax and improve the version updating process.Unit tests added: False
Estimated effort to review (1-5, lower is better): 1