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.
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
Release: new helper script to improve the release process #452
base: main
Are you sure you want to change the base?
Release: new helper script to improve the release process #452
Changes from all commits
c5c4296
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
are you deliberately avoiding the use of yq? The use of grep can be fragile in case things re-order or somehow modify... Or just a bit of in-line python, it should be installed everywhere ;-)
python -c "import yaml; print(yaml.load(open('$kata_versions_dest'), Loader=yaml.SafeLoader)['externals']['nydus-snapshotter']['version'])"
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.
In the past we discussed the problem of using yq to write yaml that will result on formatting changes, if I'm not mistaken even with python's lib. However, use yq or python just to read yaml should be acceptable.
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.
Yes, I left a note in the comments at the top of the script partly for myself about this:
I wasted a bit of my life trying to find a good solution for this, and I never found one. It's true that we could read the values out with
yq
, but in this case we end up writing it back (so, to preserve formatting, we'd still depend onsed
, even if we eliminate thegrep
). In general, I'd prefer this entire script to be python. In fact, I wrote it in python, partly to solve this problem, but then I couldn't find a solution to this issue.We also considered reading and then writing back to file all of the yaml files with one of these python libraries -- the idea being that once they're formatted by that library, future writes with that library would maintain the same formatting. But this idea has multiple problems, too (introduces a large, ugly PR across at least this repo; when the library version changes, its own formatting can change; and I think I noticed inconsistencies when testing this, too). So, unless I've missed something, grep and sed seems the least bad to me at the moment.
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's fun watching developers argue about yaml formatting while they use go which enforces
gofmt
on compile... Anyway it's not on me to judge, I'd prefer using yq at least to get the values but it's a mild suggestion.As for having this script in python, IMO it'd be more readable and writing could be performed by re string manipulation and writing the full content afterwards. The
re.sub
should be safer. Anyway I'm a pythonist so I'm probably biased so take this also as a mild suggestion.As for now the script is fine, just a bit fragile.
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.
This debate of yq/python vs sed/grep shouldn't this script any longer, IMHO. We could have it used on v0.11.0 release. Let's resume (or not) that topic in a future opportunity; for now I will approve it as is.
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.
It's most certainly going to be that "or not" case. That's why it's better to do it right from the very beginning (and use
kustomize edit set image
)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.
Could you please document in code how this works rather than a simple comment that knowing the latest commit helps? IIUC you're looking for the oldest merge commit that contains the latest change to the pre-install-payload, am I right?
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.
Yes, I added a more verbose comment above the function. I'd be grateful for a cleaner way to do this, too, if you see a way.