-
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
update airbyte-ci bump-version to push more minimal changes #34339
update airbyte-ci bump-version to push more minimal changes #34339
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @stephane-airbyte and the rest of your teammates on Graphite |
a812a10
to
eb63012
Compare
a7d3cdc
to
45165a8
Compare
airbyte-ci/connectors/pipelines/pipelines/airbyte_ci/connectors/bump_version/pipeline.py
Outdated
Show resolved
Hide resolved
updated_metadata = self.get_metadata_with_bumped_version(current_version, self.new_version, current_metadata) | ||
repo_dir_with_updated_metadata = metadata_change_helpers.get_repo_dir_with_updated_metadata( | ||
self.repo_dir, metadata_path, updated_metadata | ||
updated_metadata_str = self.get_metadata_with_bumped_version(current_version, self.new_version, current_metadata_str) |
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 totally get the reason why you mutate the metadata as string and not yaml: preserving order and comment.
I would suggest adding a yaml.safe_load(current_metadata_step)
step without assignation to make sure the yaml is still valid.
Or even better: run a MetadataValidation
step..
FYI I think this python yaml package can preserve comment and order, but I never tried it
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 call to get_current_metadata should already do that. We can't get the version number without having parsed the json succesfully.
Unless you meant that I need to run validation against the update_metadata_str ?
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.
Unless you meant that I need to run validation against the update_metadata_str ?
Yes this is what I meant
c570cff
to
837a63b
Compare
837a63b
to
131861c
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 to not block this. My remaining comments are not blocking.
…ion_to_push_more_minimal_changes
Merge activity
|
…q#34339) I noticed that `airbyte-ci connectors bump-version ` changes more things than necessary. In particular, it reserialize json, which means we were losing comments and reordering the various keys. I moved to doing a search-and-replace. In addition, I also noticed that the changelog changes could be slightly improved: I'm not increasing the space for the version (so we can accomodate 0.21.53, or 0.125.2), and adding a new line at the end of the file (which was flagged as a difference by `diff`
…q#34339) I noticed that `airbyte-ci connectors bump-version ` changes more things than necessary. In particular, it reserialize json, which means we were losing comments and reordering the various keys. I moved to doing a search-and-replace. In addition, I also noticed that the changelog changes could be slightly improved: I'm not increasing the space for the version (so we can accomodate 0.21.53, or 0.125.2), and adding a new line at the end of the file (which was flagged as a difference by `diff`
I noticed that
airbyte-ci connectors bump-version
changes more things than necessary. In particular, it reserialize json, which means we were losing comments and reordering the various keys. I moved to doing a search-and-replace. In addition, I also noticed that the changelog changes could be slightly improved: I'm not increasing the space for the version (so we can accomodate 0.21.53, or 0.125.2), and adding a new line at the end of the file (which was flagged as a difference bydiff