-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
airbyte-ci: rework RC promotion #46675
airbyte-ci: rework RC promotion #46675
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @alafanechere and the rest of your teammates on Graphite |
0296422
to
3d5bcfa
Compare
94050e0
to
ee619da
Compare
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.
Unrelated change to make the linter happy
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.
Unrelated change to make the linter happy
ee619da
to
3eb9e57
Compare
3d5bcfa
to
1d7193a
Compare
3eb9e57
to
0a6c454
Compare
1d7193a
to
f0c71ef
Compare
0a6c454
to
bd41b41
Compare
f0c71ef
to
1a998df
Compare
bd41b41
to
5db8b0d
Compare
5db8b0d
to
81f1b5c
Compare
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.
Approving so I don't block merge, but there are a few places where I'm afraid we'll swallow any errors that might happen, which will make it difficult to debug. If you could add some logging and possibly error handling too that would address my concern.
if reset_release_candidate_results.success: | ||
all_modified_files.update(await reset_release_candidate.export_modified_files(context.connector.code_directory)) |
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.
Same question as above.
if set_promoted_version_results.success: | ||
all_modified_files.update(await set_promoted_version.export_modified_files(context.connector.code_directory)) |
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.
Should we be exiting early on error here? What's the justification for moving onto the next step if this one wasn't successful?
I think we at least want some logging.
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.
I think we at least want some logging.
Errors will appear in the report as I added step results to theresults
list.
I choosed to not early exit because:
- If the step result is skipped we want to continue to the next step (e.g. a partial bump worked previously)
- If it's failing I think it's interesting to observe in the report if the next step is also failing or working.
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.
Okay cool - maybe add a comment explaining why we're okay continuing on failure?
initial_pr_creation_result = await initial_pr_creation.run(*pr_creation_args, **pr_creation_kwargs) | ||
results.append(initial_pr_creation_result) | ||
# Update changelog and update PR | ||
if initial_pr_creation_result.success: |
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.
Mind adding some logging here if it wasn't successful?
) | ||
add_changelog_entry_result = await add_changelog_entry.run() | ||
results.append(add_changelog_entry_result) | ||
if add_changelog_entry_result.success: |
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.
Same comment as above.
0189a00
to
2ddecef
Compare
2ddecef
to
8e6a057
Compare
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
What
We want to use
-rc
suffix on release candidate.It means we have to rework our promotion flow to cut release a main version when the RC is deemed stable by the system.
Promoting a RC to a main version then just means:
isReleaseCandidate
to falseHow
Rework the
airbyte-ci connectors publish --promote-release-candidate
command to:isReleaseCandidate
to falseauto-merge
label.Bonus: significant refacto on Step that modify files to have more shared logic and cleaner code.
Review guide
User Impact
/bump-version
still works following the refactoup-to-date
still works following the refacto