Update Github draft release workflow #338

psomhorst · 2024-11-12T10:38:56Z

This PR was extensively tested in a separate repo. This workflow update cleans up the workflow and more clearly splits merging a PR from merging develop. The main improvement is explicitly closing a PR using the gh CLI.

This supports either creating a release from develop -> main or from a branch associated with a PR -> main. If you select a branch with no PR associated, the merge PR job will fail.

DaniBodor · 2024-11-12T15:53:11Z

It would be easier to compare workflows if this is a change of the existing file rather than a separate file. As mentioned in #336 , we should move ahead either with that one or this one, but not both. Please let me know which you prefer.

psomhorst · 2024-11-13T09:07:12Z

Sorry for the new file, I meant to replace the original one after copying the functionality.

If this one works I would prefer this over the more complicated git commands in the current implementation. I'll test it in a fork before continuing.

DaniBodor

Great work. I think this is a much more elegant and easy to understand workflow than we had before!
I pushed one commit where I made the formatting consistent and added some names to the steps (feel free to revert if you disagree).

Apart from that a few comments/questions below.

.github/workflows/release_github.yml

psomhorst · 2024-12-18T10:55:20Z

@DaniBodor I again rewrote a significant part of the workflow. You were right in that the workflow would be partly executed but then halt if merging to develop was not possible.

There is a solution thinkable where you merge to main, but revert that if main cannot be merged to develop. This feels error-prone and could result in superfluous revert commits.

Instead, I chose to do all merging and bumping actions before pushing the results back to GitHub. If at any stage merging to main or from main to develop fails, the repo will not be pushed and the action fails.

By merging the PR it is automatically closed. This does not remove the branch, so that is done separately in cleanup.

I also rewrote/added checks. They now:

check whether the branch is not main
if the branch is not develop:
- check whether a PR exists
- check whether the base of the PR is main
- check whether the PR is mergeable
- check whether the GitHub checks for the PR pass

I tested all this in a fork of the repo. The situations I tested:

merge from branch that has no PR fails
merge from branch that has develop as base fails
merge from branch that has a conflict with develop fails (because it can't merge into develop)
merge from branch that has a conflict with main fails
merge from develop that has a conflict with main fails
merge from branch when PR checks are still running waits for the checks to finish
merge from branch when a PR check fails fails
merge from branch that has no conflicts with passing checks succeeds
merge from branch that develop without conflicts succeeds

Lastly I removed the CITATION file validation. This failed for a reason I could not explain. Also, this validation is done for every PR using another action. This felt superfluous at this stage.

DaniBodor

Even better and clearer changes now!
And great that you tested all those possibilities. I can't think of other situations now that are not covered.
I still added a few things to address below :)

.github/workflows/release_github.yml

DaniBodor · 2024-12-18T14:37:02Z

.github/workflows/release_github.yml

+        if: ${{ github.ref_name != 'develop' }}
+        run: |
+          gh pr checks ${{ github.ref_name }} --watch --fail-fast
+          gh pr checks ${{ github.ref_name }} --json state -q 'if . | length == 0 then "No checks found." elif map(.state == "SUCCESS") | all then "All checks passed" else error("Not all checks passed") end'            


is there a way to set the if length==0 then succeed?
I think it's rare, but can imagine minor changes that trigger no checks (e.g. because most checks are only triggered on certain file type changes). In that case, it seems like there are no ways to allow a release...

Yes, this is possible (I implemented it at some point), but there are two reasons I didn't put it in the last version.

The main reason is that if you're fast enough after pushing changes to a PR, you can trigger the action before GitHub has detected that it should do checks. It will give an error that there were no checks for this branch. This happened to me once during testing the workflows.
If we would make it so it skips this when there are no checks, it could skip it unintentionally.

The second reason is that it's quite verbose, something like (thanks to ChatGPT):

if [ $exit_code -ne 0 ]; then # Look for the specific error message if echo "$output" | grep -q "no checks reported on "; then echo "Caught the expected error: no checks reported on the branch" # Handle the specific error here else # Re-raise other errors echo "An unexpected error occurred: $output" exit $exit_code fi else echo "Command succeeded: $output" # Handle success case here fi

I don't think it's possible no check is triggered in the current repo. If there will be such a case, it is easy to add a simple check that always succeeds just to make sure that there is always a check to make this succeed.

.github/workflows/release_github.yml

DaniBodor · 2024-12-18T14:41:46Z

.github/workflows/release_github.yml

+      matrix:
        os: ["ubuntu-latest"]


I don't think there's anything specific to linux in this package, right? Maybe it would be nice to add OSX and windows to the matrix as well, to ensure cross-platform compatibility?

Suggested change

matrix:

os: ["ubuntu-latest"]

matrix:

os: ["ubuntu-latest", "windows-latest", "macos-latest"]

Yes! And no. But mostly yes.

Doing all versions on all platforms is (unnecessarily?) expensive and slow. Should we test the lowest supported version on all platforms, and up to the newest supported version on linux?

I think given that it's only done rarely (upon release) it's fine to have a large matrix.
If you feel it's overkill, I would leave it like it was. I think it would create more confusion than clarity to create some versions on some platforms and all versions on another (plus would again become quite verbose).

.github/workflows/release_github.yml

DaniBodor · 2024-12-18T15:09:49Z

.github/workflows/release_github.yml

+        if: ${{ github.ref_name != 'develop' }}
+        run: gh pr view ${{ github.ref_name }} --json baseRefName -q 'if .baseRefName == "main" then "PR base is main" else error("PR base is not main") end'
+
+      - name: Check if PR is mergeable


Shouldn't this check happen here, also if the release branch is develop? See also this comment

If the release branch is develop, there is no PR. This only works on PRs.

If the release branch is develop, there are no checks. These checks are done before PRs are merged into develop. As long as that is always the case, there are no checks required for develop.

Alternatively, we could also create a PR for merging develop into main. That would simplify this workflow a bit, but also add a step when creating a release. I don't dislike the idea, though. The workflow for making a release would change to:

create PR (either from branch or develop)

trigger action

If the release branch is develop, there are no checks. These checks are done before PRs are merged into develop. As long as that is always the case, there are no checks required for develop.

I think there is no formal restriction for develop to have conflicts to main. This could either happen due to non-careful PR review, or e.g. after a hotfix was made directly to main.

Alternatively, we could also create a PR for merging develop into main. That would simplify this workflow a bit, but also add a step when creating a release. I don't dislike the idea, though. The workflow for making a release would change to:

create PR (either from branch or develop)

trigger action

I also like this workflow. Also makes things more consistent

Agreed. I implemented this in 379316b.

in fact, the documentation already says to this: step 0 😉

DaniBodor · 2024-12-18T15:15:36Z

merge from branch that has a conflict with develop fails (because it can't merge into develop)

On second thought, is this what we want?
Assuming you make a hotfix directly to main that conflicts with develop, you still want to be able to release it, right? The issue can then be resolved in develop, rather than having to spend time ensuring that the conflict is resolved in the hotfix branch.
Is the protocol in this case to move to a manual release (if so, that should be clear from the resulting error message)?

I think the most elegant solution (which is maybe too much effort to implement for the rare occurrence) would be to have an optional override conflict to develop setting that is defaulted to off. This would ensure that the issue is clearly flagged when someone tries to release a develop conflict (which would be a bit hidden otherwise), but still allows merging if need be.

DaniBodor · 2024-12-18T15:17:34Z

if the branch is not develop:

check whether a PR exists

check whether the base of the PR is main

check whether the PR is mergeable

check whether the GitHub checks for the PR pass

why only do all this if it's a hotfix branch, but not develop?

psomhorst · 2024-12-18T15:39:05Z

merge from branch that has a conflict with develop fails (because it can't merge into develop)

On second thought, is this what we want? Assuming you make a hotfix directly to main that conflicts with develop, you still want to be able to release it, right? The issue can then be resolved in develop, rather than having to spend time ensuring that the conflict is resolved in the hotfix branch. Is the protocol in this case to move to a manual release (if so, that should be clear from the resulting error message)?

I think the most elegant solution (which is maybe too much effort to implement for the rare occurrence) would be to have an optional override conflict to develop setting that is defaulted to off. This would ensure that the issue is clearly flagged when someone tries to release a develop conflict (which would be a bit hidden otherwise), but still allows merging if need be.

There would be rare occasions where this is an issue. That is when develop contains changes that overlap with the hotfix. For example, when a fix is already merged with develop, but we don't want to make a full release of all changes to develop yet, so we want to merge a slightly different fix to main.
We could then manually merge the PR into main and create a GitHub release.

This is so rare (hasn't occurred yet) that I don't think it's worth complicating this workflow for.

psomhorst · 2024-12-18T15:41:30Z

if the branch is not develop:
* check whether a PR exists

* check whether the base of the PR is main

* check whether the PR is mergeable

* check whether the GitHub checks for the PR pass
why only do all this if it's a hotfix branch, but not develop?

This is a choice we made a while ago. I think our idea was: create PR, merge into develop (times x), then merge develop into main. Later we thought of hotfixes that should bypass this, creating the distinction.

DaniBodor · 2024-12-18T16:04:06Z

merge from branch that has a conflict with develop fails (because it can't merge into develop)

On second thought, is this what we want? Assuming you make a hotfix directly to main that conflicts with develop, you still want to be able to release it, right? The issue can then be resolved in develop, rather than having to spend time ensuring that the conflict is resolved in the hotfix branch. Is the protocol in this case to move to a manual release (if so, that should be clear from the resulting error message)?
I think the most elegant solution (which is maybe too much effort to implement for the rare occurrence) would be to have an optional override conflict to develop setting that is defaulted to off. This would ensure that the issue is clearly flagged when someone tries to release a develop conflict (which would be a bit hidden otherwise), but still allows merging if need be.

There would be rare occasions where this is an issue. That is when develop contains changes that overlap with the hotfix. For example, when a fix is already merged with develop, but we don't want to make a full release of all changes to develop yet, so we want to merge a slightly different fix to main. We could then manually merge the PR into main and create a GitHub release.

This is so rare (hasn't occurred yet) that I don't think it's worth complicating this workflow for.

Is it at least worthwhile adding a note at the point where the action failed that this is the reason and that the merge+release should be done manually? Or is that also overkill?

psomhorst · 2024-12-18T16:11:36Z

Is it at least worthwhile adding a note at the point where the action failed that this is the reason and that the merge+release should be done manually? Or is that also overkill?

Good suggestion. Added in 388021a.

- Check for a main branch first - Separate jobs for merging develop or a PR - Use gh CLI for merging PR - Bump version as separate job - Other small improvements

Co-authored-by: Dani Bodor <d.bodor@esciencecenter.nl>

When any step in this job fails, the repo is never pushed back. This should make sure the any failure will result in no actual changes to the repo.

This was referenced Nov 12, 2024

Mark GitHub release branches as merged instead of closed #335

Closed

style: formatting non-python files #336

Merged

DaniBodor linked an issue Nov 14, 2024 that may be closed by this pull request

Mark GitHub release branches as merged instead of closed #335

Closed

psomhorst force-pushed the release_suggestion_psomhorst branch 3 times, most recently from 1439d15 to 4896caf Compare December 16, 2024 13:34

psomhorst changed the title ~~Github release suggestion~~ Update Github draft release workflow Dec 16, 2024

psomhorst marked this pull request as ready for review December 16, 2024 13:37

psomhorst requested a review from DaniBodor December 16, 2024 13:38

DaniBodor reviewed Dec 17, 2024

View reviewed changes

psomhorst changed the base branch from develop to main December 17, 2024 14:02

psomhorst changed the base branch from main to develop December 17, 2024 14:02

psomhorst force-pushed the release_suggestion_psomhorst branch 3 times, most recently from def8a73 to d063aa4 Compare December 18, 2024 10:49

psomhorst requested a review from DaniBodor December 18, 2024 10:55

DaniBodor requested changes Dec 18, 2024

View reviewed changes

DaniBodor reviewed Dec 18, 2024

View reviewed changes

DaniBodor approved these changes Dec 18, 2024

View reviewed changes

Update github release action

0ed4b8f

- Check for a main branch first - Separate jobs for merging develop or a PR - Use gh CLI for merging PR - Bump version as separate job - Other small improvements

DaniBodor and others added 10 commits December 18, 2024 20:29

formatting and step names

ea4bfa4

Remove global option for git config

cf0c4fb

Co-authored-by: Dani Bodor <d.bodor@esciencecenter.nl>

Combine all repo changes into a single job and rewrite checks

4def6e2

When any step in this job fails, the repo is never pushed back. This should make sure the any failure will result in no actual changes to the repo.

style: automatically remove trailing whitespaces

37f6350

Set specific version of bump-my-version

9175be7

Install bump-my-version version from pyproject.toml

78b6cdc

Fix step names and blank lines

7f77d61

Clarify tidying job

fc06cf5

Remove skipping checks if ref is develop

c8ba98b

Add error message when failing to merge with develop

8bed368

psomhorst force-pushed the release_suggestion_psomhorst branch from 388021a to 8bed368 Compare December 18, 2024 19:29

psomhorst merged commit 2a3eaf8 into develop Dec 18, 2024
4 checks passed

psomhorst deleted the release_suggestion_psomhorst branch December 18, 2024 19:39

This was referenced Dec 19, 2024

ci: automatically deploy docs neurogym/neurogym#102

Merged

ci: streamline GitHub release neurogym/neurogym#103

Merged

Update Github draft release workflow #338

Update Github draft release workflow #338

Uh oh!

Conversation

psomhorst commented Nov 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaniBodor commented Nov 12, 2024

Uh oh!

psomhorst commented Nov 13, 2024

Uh oh!

DaniBodor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

psomhorst commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaniBodor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DaniBodor Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psomhorst Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DaniBodor Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

psomhorst Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

DaniBodor Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaniBodor Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

psomhorst Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaniBodor Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

psomhorst Dec 18, 2024

Choose a reason for hiding this comment

Uh oh!

DaniBodor Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaniBodor commented Dec 18, 2024

Uh oh!

DaniBodor commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

psomhorst commented Dec 18, 2024

Uh oh!

psomhorst commented Dec 18, 2024

Uh oh!

DaniBodor commented Dec 18, 2024

Uh oh!

psomhorst commented Nov 12, 2024 •

edited

Loading

DaniBodor left a comment •

edited

Loading

psomhorst commented Dec 18, 2024 •

edited

Loading

DaniBodor left a comment •

edited

Loading

DaniBodor Dec 18, 2024 •

edited

Loading

DaniBodor Dec 18, 2024 •

edited

Loading

psomhorst Dec 18, 2024 •

edited

Loading

DaniBodor Dec 18, 2024 •

edited

Loading

DaniBodor commented Dec 18, 2024 •

edited

Loading