Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

portersrc
Copy link
Member

@portersrc portersrc commented Sep 26, 2024

During the CoCo release process, there are a couple of tedious steps in the operator repo that can be automated. The script in this PR attempts to do that.

We just used this to help with the v0.10.0 release. The checklist is here. This script helps with steps 4 and 5.

This script has been used for v0.9.0 and now v0.10.0, so it's been run a couple of rounds and been found useful enough to submit for review.

@portersrc portersrc requested a review from a team as a code owner September 26, 2024 04:01
@wainersm
Copy link
Member

wainersm commented Oct 3, 2024

Hi @portersrc !

That's a great idea to have a script to help on the release. Also it's always nice to see (and learn) new different ways to doing things in bash!

@portersrc
Copy link
Member Author

Hi @wainersm thanks for the helpful feedback. I've tried to address each one.


# multi-line grep to find the nydus-snapshotter version in kata's version.yaml
# then awk/tail/tr and string manipulation to sand it down.
nydus_version_kata="$(grep -zoP "nydus-snapshotter.*\n.*description.*\n.*url.*\n.*version.*" ${kata_versions_dest} | awk '{print $2}' | tail -n1 | tr -d '\0')"
Copy link
Contributor

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'])"

Copy link
Member

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.

Copy link
Member Author

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:

# Note: This script intentionally uses sed. yq (or even python libraries like   
# "yaml" or "ruamel.yaml") do not preserve yaml formatting.                     

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 on sed, even if we eliminate the grep). 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.

Copy link
Contributor

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.

hack/release-helper.sh Outdated Show resolved Hide resolved
#
function update_prereqs() {
# grab the hash for the latest commit on the pre-install-payload folder
# in github (which requires figuring out the hash of the merge)
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is definitely a step to the right direction although there are many places with unquoted variables. Would be nice to treat them (you can use shellcheck to find them all ;-) )

@portersrc
Copy link
Member Author

portersrc commented Oct 7, 2024

Thanks @ldoktor for the review! I've addressed the minor things and left a couple comments open for you.

hack/release-helper.sh Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants