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.

Sign up for GitHub

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

Jump to bottom

(chore) document and refactor release scripts #8078

Merged
merged 7 commits into from
Jan 12, 2022
Merged

(chore) document and refactor release scripts #8078

merged 7 commits into from
Jan 12, 2022

Conversation

kikito
Copy link
Member

@kikito kikito commented Nov 15, 2021

  • chore(scripts) add instructions to some steps
  • refactor(scripts) extract check_requirements to common.sh
  • chore(scripts) make-prererelease-release works without flags
  • chore(scripts) pass all global arguments to usage function in release scripts
  • refactor(scripts) move duplicated release steps to common.js

Summary

SUMMARY_GOES_HERE

Full changelog

  • [Implement ...]
  • [Add related tests]
  • ...

Issues resolved

Fix #XXX

@kikito
chore(scripts) add instructions to some steps
5df26e6
@kikito
refactor(scripts) extract check_requirements to common.sh
3bc6433
@kikito
chore(scripts) make-prererelease-release works without flags
25e5c79 
The other scripts use positional arguments, this script should use the same.
@kikito
chore(scripts) pass all global arguments to usage function in relea…
41037e2 
…se scripts
@kikito
refactor(scripts) move duplicated release steps to common.js
b99d1dc 
There have been some changes in the scripts as well:

* Some steps were renamed (e.g `luarocks` became `publish_luarock`)
* Steps that are now partially done by the CI only have the human part
  left (review the machine-generated prs)
@kikito kikito requested a review from gszr November 16, 2021 14:51
@jschmid1 jschmid1 added the chore Not part of the core functionality of kong, but still needed label Nov 22, 2021
@fffonion
Copy link
Contributor

@kikito Is there a specific parts needs to be reviewed other than trying this out locally (which I assume you have already did)?

flrgh
flrgh reviewed Nov 30, 2021
View reviewed changes
scripts/common.sh Outdated Show resolved Hide resolved
@kikito
Copy link
Member Author

kikito commented Dec 2, 2021

@fffonion that is in fact a very good point. There is no "dry-run" mode for these scripts so "manually trying" is often not even an option.

I have tried some of the steps manually, but I have not tried all the combinations I am afraid T__T

jschmid1
jschmid1 reviewed Dec 21, 2021
View reviewed changes
Copy link
Contributor

@jschmid1 jschmid1 left a comment

Choose a reason for hiding this comment

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

looks good overall, hard to actually test though without a proper dryrun mode.

scripts/common.sh Outdated Show resolved Hide resolved
scripts/common.sh Outdated Show resolved Hide resolved
scripts/common.sh
CONFIRM "Press Enter to push the branch and open the release PR" \
"or Ctrl-C to cancel."

set -e
Copy link
Contributor

Choose a reason for hiding this comment

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

an overall comment on the set -e and SUCCESS interaction. A nicer and more userfriendly way would be to use traps. This maybe out of scope for PR but I'll leave it here for inspiration.

Copy link
Member Author

Choose a reason for hiding this comment

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

I admit that I have never used trap in such a way (I try to avoid bash script when I can ...).

From what I have read, it seems that we would still need set -e (to produce the signal that then would be captured by trap).

Let's call it out of scope for now, it seems a bit ... philosophical as well (one global rescue function vs lots of small traps?)

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a live conversation about this.

@gszr , I think there must be a particular reason why set -e is set/unset on every action instead of setting it once globally. Is there? What is your opinion of using trap to recover? At the very least we should be able to print "the command blablabla failed with blebleble"

kikito and others added 2 commits January 10, 2022 20:48
@kikito @jschmid1 @flrgh
refactor(scripts) improve usage over which
6624eb1 
Co-authored-by: Joshua Schmid <joshua.schmid@konghq.com>
Co-authored-by: Michael Martin <flrgh@protonmail.com>
@kikito
refactor(scripts) introduce yesno script function
87125eb
@kikito kikito force-pushed the chore/release-scripts branch from d381c18 to 87125eb Compare January 10, 2022 19:49
@kikito kikito merged commit c97261d into master Jan 12, 2022
@kikito kikito deleted the chore/release-scripts branch January 12, 2022 12:49
kikito added a commit that referenced this pull request Mar 1, 2022
@kikito
chore(scripts) include jenkins steps
7ea02bc 
Our jenkins server expects and uses some steps in scripts/make-patch-release that were removed in #8078. This commit adds them back as a temporary way of releasing 2.8.0
kikito added a commit that referenced this pull request Mar 3, 2022
@kikito
chore(scripts) include jenkins steps
9c07d24 
Our jenkins server expects and uses some steps in scripts/make-patch-release that were removed in #8078. This commit adds them back as a temporary way of releasing 2.8.0
kikito added a commit that referenced this pull request Mar 3, 2022
@kikito
chore(scripts) restore update_docker function
3a0429c 
This function was removed accidentally in #8078
kikito added a commit that referenced this pull request Apr 8, 2022
@kikito
chore(scripts) add missing update_docker function
c2b80cd 
This function was removed by mistake on a script refactor (#8078).

It was cherry-picked back into the master branch (#8495) but not to the
release/2.8.x branch. It is not needed for kong to work, but it might be
needed in order to do the post-release steps of a hypothetical 2.8.2
release.
@kikito kikito mentioned this pull request Apr 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flrgh flrgh flrgh left review comments

@jschmid1 jschmid1 jschmid1 approved these changes

@gszr gszr Awaiting requested review from gszr

Assignees
No one assigned
Labels
chore Not part of the core functionality of kong, but still needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kikito @fffonion @flrgh @jschmid1