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

Fix release workflows #395

Merged
merged 12 commits into from
May 8, 2024
Merged

Fix release workflows #395

merged 12 commits into from
May 8, 2024

Conversation

gjtorikian
Copy link
Collaborator

The bad news is I made a slight error in #374: I needed to provide a GitHub token to create a release.

The good news is that now that the workflow is in main, I can make any adjustments necessary here in the PR.

I also noticed a bunch of warnings stemming from the (deprecated!) https://github.com/actions-rs/toolchain action:

Screenshot 2024-05-01 at 12 53 54

I'm actually not sure why I included this step, because it's not at all necessary to have Rust installed for this process.

@digitalmoksha
Copy link
Collaborator

Nice!!

Copy link
Contributor

github-actions bot commented May 1, 2024

Command Mean [ms] Min [ms] Max [ms] Relative
./bench.sh ./comrak-90ee48d 327.5 ± 2.2 323.1 332.0 2.88 ± 0.04
./bench.sh ./comrak-main 329.3 ± 2.9 326.4 340.1 2.90 ± 0.04
./bench.sh ./pulldown-cmark 113.7 ± 1.2 111.9 117.0 1.00
./bench.sh ./cmark-gfm 121.6 ± 1.4 119.6 125.8 1.07 ± 0.02
./bench.sh ./markdown-it 500.4 ± 6.9 493.0 526.7 4.40 ± 0.08

Run on Wed May 1 19:59:27 UTC 2024

@gjtorikian
Copy link
Collaborator Author

It works! Kind of.

But, the part of the process that autogenerates the changelog failed, because it can't push to the branch:

Waiting for status checks to finish for 'push-action/8914509187/27943-4815-12997' ...

Configuration:
    interval: 30 seconds
    timeout: 15 minutes
    required status checks: ['no_features_build_test (beta)', 'no_features_build_test (stable)', 'lockfile', 'build_spec (beta)', 'build_spec (stable)', 'build_test (beta)', 'build_test (stable)']
        of which are:
            GitHub Action-related: 0
            Third-party checks: 0

All required GitHub Actions jobs complete!
Waiting for status checks to finish for 'push-action/8914509187/27943-4815-12997' ... DONE!

Pushing 'push-action/8914509187/27943-4815-12997' -> 'origin/main' ...
branch 'main' set up to track 'origin/main'.
Switched to a new branch 'main'
HEAD is now at 64c3011 [auto-docs][skip test]: update changelog
remote: error: GH006: Protected branch update failed for refs/heads/main.        
remote: error: 7 of 7 required status checks are expected.        
To https://github.com/kivikakk/comrak.git
 ! [remote rejected] main -> main (protected branch hook declined)
error: failed to push some refs to 'https://github.com/kivikakk/comrak.git'

Removing temporary branch 'push-action/8914509187/27943-4815-12997' ...
Removing temporary branch 'push-action/8914509187/27943-4815-12997' ... DONE!

Since the main branch is protected by required status checks, the automation can't automatically perform the push to that branch. I like having this push, rather than a PR, because it occurs before the tag, which ensures that the latest changelog is included within the latest git tag/release.

To work around this in my other projects, I often rely on https://github.com/CasperWA/push-protected. It explains the token problem quite clearly:

The reason why you can not use the GITHUB_TOKEN secret when pushing to a branch that is protected by required status checks, is that using this as authentication does not trigger any webhook events, such as 'push', 'pull_request', etc. This event trigger is a MUST for starting the required status checks on the temporary branch, which are necessary to run in order to be able to push the changes into the desired branch.

So, in short, we'd need a PAT generated with the appropriate permissions, stored as a secret in this repo. Here are the permissions for the one I use:

Screenshot 2024-05-01 at 13 48 50

Let me know if this makes sense or if there's another alternative you'd like to see!

@digitalmoksha
Copy link
Collaborator

digitalmoksha commented May 1, 2024

The way I've handled it in a project is by using a release PR. You create a PR that bumps the version number. On merging with main, CI checks if the version has changed (or the PR title has the word RELEASE in it), and it then runs the extra jobs to do further tests, create the release and push to rubygems.

If that process were used, the jobs running on the branch could do the same version/title check, and push an update to the changelog, which would get merged before the release is cut.

One possibility to keep from having to add a PAT.

ADDED: granted, I did this with GitLab CI, so the job language is different - don't know if it's possible here.

@gjtorikian
Copy link
Collaborator Author

Ooh, that makes sense. I'll chew on that and work with it. Thanks for the suggestion!

@kivikakk
Copy link
Owner

kivikakk commented May 2, 2024

Thanks so much for your work here, @gjtorikian!! And likewise @digitalmoksha for chipping in with your experience.

(I am of course happy to go either way — if you run out of steam looking into doing it with a release PR, Garen, it doesn't bother me to use a PAT like you do elsewhere.)

@gjtorikian
Copy link
Collaborator Author

Well, I usually copy paste this PAT technique across all my projects, but it is a significant annoyance: these PATs must have an expiry date, and so require attention and upkeep. I’d rather poke around on how I can get the PR process working since it involves even less human involvement!

@digitalmoksha
Copy link
Collaborator

Don't know if it would be helpful or not, but here's the GitLab CI configuration we use for the GLFM gem, with the release trigger being in a shared component (or action)

https://gitlab.com/gitlab-org/components/gem-release/-/blob/main/templates/gem-release/template.yml?ref_type=heads#L28

.gem-release-default-rules:
  rules:
    - if: '$CI_PIPELINE_SOURCE == "push" && $CI_COMMIT_BRANCH == $CI_DEFAULT_BRANCH'
      changes: ["$[[ inputs.file_pattern_to_trigger_release ]]"]
    - if: '$CI_MERGE_REQUEST_TITLE =~ /RELEASE/ && "$[[ inputs.dry_run ]]" == "true"'
    - if: '$CI_MERGE_REQUEST_TITLE =~ /RELEASE/'
      when: manual

.dry-run-rules:
  rules:
    - if: '"$[[ inputs.dry_run ]]" == "true"'
      when: never

The other nice little bit is the use of dry_run, which will run everything except tagging and pushing the gem. So you can put "RELEASE" in your PR title, and dry_run in the ci file, and fully test the pipeline outside of the main branch.

@gjtorikian
Copy link
Collaborator Author

After much chagrin and gnashing of teeth, I ran into another problem with the automation: PRs that are opened by the GitHub Actions bot do not automatically run workflows. Therefore, they can't be automerged, because the required test suite never executes. A solution to this is to use a PAT with the permissions listed above, which is exactly what I am trying to avoid.

With that in mind, here's the proposed release workflow:

  1. When an admin is ready to make changes, they can manually kick off the Release workflow (same as before)
  2. This creates a new PR with the changelog and README auto-generated (@digitalmoksha's suggestion)
  3. The PR is created with the label release (my suggestion)

At this point, one can push updates to the changelog if need be. But the admin needs to manually merge this release PR. When that occurs, the rest of the process kicks off: a git tag is created, a GitHub release is made, and the lib is pushed to crates.io.

@gjtorikian
Copy link
Collaborator Author

I'm more than moderately confident this will work, as the PR changelog is the only new piece that changed since the last run through. I'm happy with this PR being merged as is, or take any feedback!

@kivikakk
Copy link
Owner

kivikakk commented May 8, 2024

LGTM! Thank you so much!

I've updated changelog.txt so far, so the format will match going forward. (I'm not adding v to existing entries, so the changelog entries all match their corresponding tags; I'm happy with this PR doing vX.Y.Z tags instead of the current X.Y.Z tags, I should've done that all along.)

@kivikakk kivikakk merged commit 82d15b5 into main May 8, 2024
25 checks passed
@kivikakk kivikakk deleted the fix-release-workflows branch May 8, 2024 18:38
kivikakk added a commit that referenced this pull request May 8, 2024
@kivikakk
Copy link
Owner

kivikakk commented May 8, 2024

After much chagrin

asqhjkcrflcxhsqkj, no kidding 😅

Thank you again — I've updated RELEASE_CHECKLIST.md (9962cdf) accordingly.

@digitalmoksha
Copy link
Collaborator

So with ya'lls permission, wanted to try to release a 0.24.0 (or 0.23.1) with the new wiki link support.

I just want to clarify the first step, Bump version in Cargo.toml.

So update Cargo.toml and commit it directly into main?

Or run the release workflow, which creates the PR, then commit the version update on that branch?

@kivikakk
Copy link
Owner

I think the latter should work just as well (and would therefore be preferable!).

And it should be 0.24.0, since we've modified the API in a backwards incompatible way: anyone doing an exhaustive match on a NodeValue will have to handle NodeValue::WikiLink.

@digitalmoksha
Copy link
Collaborator

Just tried - at the moment it doesn't work, which makes sense - the PR title, changelog comparison, and the branch name are built using the new version number. See #414

So I will push the version to main

@digitalmoksha
Copy link
Collaborator

Of course main is protected. Creating PR #415

@digitalmoksha
Copy link
Collaborator

#416 was created by release workflow. But I don't have permission to merge it without all checks passing.

I removed the [skip test] in the PR title. But I don't see how I can trigger the jobs to run again, at least not without pushing another commit. 🤔 Though I'm probably overlooking something.

Going to hold off for the moment.

@kivikakk
Copy link
Owner

Closed and re-opened the PR per the PR text; hopefully should do it.

@digitalmoksha
Copy link
Collaborator

Yep, that's what happens when trying to do something when enough coffee hasn't been consumed - you gloss over clearly written text. 🤦

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