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

Add workflow to release webhook in rancher/charts and rancher/rancher #449

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

tomleb
Copy link
Contributor

@tomleb tomleb commented Jul 12, 2024

Issue rancher/rancher#46704

What is this

This adds two workflows that can be triggered manually to bump webhook RCs to rancher/charts and rancher/rancher.

Creating a new release in rancher/charts and rancher/rancher will be simple:

  1. Go to the rancher/webhook repo
  2. Click on the Actions tab
  3. On the left sidebar, click on the workflow that you want to run.
  4. Enter the necessary input. Example in the image below.
    image
  5. Click Run workflow

This will do the following.

charts

Create a new branch off of the branch you input, make some changes to bump webhook, push to rancher/charts directly (more details below), create a pull request to the target branch. You can see an example here: rancher/charts#4211.

rancher

Create a new branch off of the branch you input, update webhook version in build.yaml, run go generate, push rancher/rancher, create a pull request to the target branch. You can see an example here: tomleb/rancher-rancher#8.

Support

Currently, this supports the following:

  • Bumping from one RC to another
  • Bumping from one RC to unRC

It does not yet support adding an entirely new version of webhook. I'd be okay with implementing it for this PR if asked.

Benefits

  • No need to open a terminal to do this work (tedious)
  • Faster to create PRs: I made sure to include some relevant information to the PR description when they are opened. Here's an example:
    image. I'm lazy so I'd rather not have to fetch this information every time like I've been doing and it should make it easier for reviewer as well to review this.
  • Easier to review due to the above PRs containing lots of information and links
  • This will also mean that we're going to need only 2 people for an RC bump because the PR will be created by the bot. So the person running the workflow + another person will be enough to review a bump.

Pushing to rancher/charts & rancher/rancher

EIO provided us with a GH app that can be used (through vault) to create tokens for rancher/rancher and rancher/charts. This allows us to push to those repositories directly and create PRs.

Credits

A lot of it was taken from the fleet team:

Some notable changes:

  • Removed support from unRC and creating a new release entirely. Mostly to simplify, let's start with just bumping RCs which is what we end up spending most of our times on.
  • Removed the need to input the chart version number (104.0.0 in 104.0.0+up0.5.0-rc13). Instead, we just reuse the one for that release.
  • Removed the usage of the peter-evans/create-pull-request action. This has been explicitly rejected by the security team so we're not allowed to use it. Instead, using gh cli tool is good enough.
  • More information in the PR description generated as mentioned above.

@tomleb tomleb requested a review from a team as a code owner July 12, 2024 19:28
@tomleb tomleb marked this pull request as draft July 12, 2024 19:29
.github/workflows/release-charts.yaml Outdated Show resolved Hide resolved
.github/workflows/release-rancher.yaml Outdated Show resolved Hide resolved
.github/workflows/release-rancher.yaml Outdated Show resolved Hide resolved
.github/workflows/release-charts.yaml Outdated Show resolved Hide resolved
@ericpromislow
Copy link
Contributor

Regarding the env vars, once this is out of draft and ready to play with I'll see if I can trigger an exploit

Comment on lines +63 to +66
user_id=$(gh api "/users/$APP_USER" --jq .id)"
git config --global user.name "$APP_USER"
git config --global user.email "${user_id}+${APP_USER}@users.noreply.github.com>"
env:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adapted from https://github.com/actions/create-github-app-token, unsure how necessary this is, probably not, but I guess that's probably the "idiomatic" values to put there so why not

@tomleb
Copy link
Contributor Author

tomleb commented Sep 20, 2024

Updated the workflow so they use a github apps credential provided by EIO instead. No need for a fork anymore, can directly push to our repos.

@tomleb tomleb marked this pull request as ready for review September 20, 2024 12:13
Comment on lines 31 to 36
# Validate the given webhook version (eg: 0.5.0-rc.13)
if ! grep -q "${PREV_WEBHOOK_VERSION}" ./packages/rancher-webhook/package.yaml; then
echo "Previous Webhook version references do not exist in ./packages/rancher-webhook/. The content of the file is:"
cat ./packages/rancher-webhook/package.yaml
exit 1
fi
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could also avoid requiring the old version of webhook and instead fetch it directly from that same file. I'd be okay with that. This way you'd just need to know the version you want to go to.

@prachidamle prachidamle requested a review from a team September 20, 2024 19:49
@tomleb tomleb changed the base branch from release/v0.5 to main September 23, 2024 19:59
Comment on lines +49 to +51
# DAPPER_MODE=bind will make sure we output everything that changed
DAPPER_MODE=bind ./.dapper go generate ./... || true
DAPPER_MODE=bind ./.dapper rm -rf go
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any benefit in using dapper for this? Perpetuating the use of dapper seems suboptimal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this on slack but for visibility:

go generate might use a few tools like go and controller-gen. Those tools version are currently only defined in the Dockerfile for dapper in r/r. I want to be able to generate using the right tools, so using dapper for now is convenient because we don't need to care about those tools, no need to keep maintaining and syncing them with r/r, etc.

I don't really have bandwidth for making a change in r/r to allow that (eg: by extracting the versions of tools outside).

.github/workflows/scripts/release-message.sh Outdated Show resolved Hide resolved
.github/workflows/release-charts.yaml Outdated Show resolved Hide resolved
pmatseykanets
pmatseykanets previously approved these changes Oct 3, 2024
make .dapper

# DAPPER_MODE=bind will make sure we output everything that changed
DAPPER_MODE=bind ./.dapper go generate ./... || true
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to go generate everything here or intent was only to call go run ./pkg/codegen/buildconfig/...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could extract the commands that go generate runs but I think it's more future proof to use go generate directly. The tradeoff is that this takes longer to run (which also means preventing other jobs to run).

Copy link
Contributor

Choose a reason for hiding this comment

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

It also has potential of failing doing the irrelevant work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave as is for now.

Copy link
Contributor

@nflynt nflynt left a comment

Choose a reason for hiding this comment

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

Looks mostly okay to me. If we can make the new RC handling a tiny bit more robust I think I have no other changes to add. Mostly we want to make it impossible/difficult for newly onboarded engineers to accidentally blast build artifacts. :)

.github/workflows/scripts/release-against-charts.sh Outdated Show resolved Hide resolved
@tomleb
Copy link
Contributor Author

tomleb commented Oct 16, 2024

Updated the release against charts script to support going from stable release to new rc (eg: v0.5.2 to v0.5.3-rc.1). I have tested it locally and that seems to work fine.

nflynt
nflynt previously approved these changes Oct 16, 2024
Copy link
Contributor

@nflynt nflynt left a comment

Choose a reason for hiding this comment

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

This looks good to me. Now that the RC guards are in place, nothing's really jumping out. We can improve it further as we go. 👍

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.

4 participants