-
Notifications
You must be signed in to change notification settings - Fork 25
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
Add update script to fetch new kn version and update formulas #138
Conversation
The update action showcase on my fork: dsimansk#1 |
Failed CI check is unrelated to this changes, being addressed by #137. |
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.
Thanks, looks great! Some minor nits inline, as usual ;-)
Co-authored-by: Roland Huß <[email protected]>
hack/update-taps.sh
Outdated
|
||
# Generate file based on the inlined HEREDOC template. | ||
function generate_tap_file() { | ||
out=$1 |
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.
you could mark all those variables as local
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.
Well, I had that though, but then shellcheck
wanted me to split declaration and I was too lazy to do 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.
I'll change it NP.
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.
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.
strange, wonder what the reasoning of shellcheck is for insisting of having declaration and initialization in two statements and why it doesn't complain about global variables ;-)
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.
Looks good to me with one minor nit, but I don't insist on that :-)
/approve
/lgtm
/hold
@rhuss updated with /unhold |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dsimansk, rhuss The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Thanks again for taking care! |
I've done a bit of overhaul on the script we have for manual update.
My idea is to drive the execution through GH actions primarily, although it's runable locally in current format. Create one GH action with manual run dispatch trigger. And periodical trigger on a day after patch release job is executed, give or take simply put once per week.
The logic of fetching version is taken from client and usual git parsing tag approach.
Then generating the file based on template. Also old file is always generated to get rid of
sed
shenanigans. :)It can update to current patch version release and bump to new 1.y.z minor every release cycle.
It can't update previous patch versions though. I could add a logic to always check and reconcile N-2 version. But seemed like a corner case to me.
Superseding: https://github.com/knative/homebrew-client/blob/main/hack/update-versions.sh
/cc @rhuss