EIT-ALIVE / eitprocessing

Software for electrical impedance tomography data loading, visualization and processing
https://eit-alive.github.io/eitprocessing/
Apache License 2.0
5 stars 1 forks source link

ci: github release action #242

Closed DaniBodor closed 5 months ago

DaniBodor commented 5 months ago

This PR creates a workflow to automate the release process.

As currently implemented, this workflow can be triggered manually from the Actions tab and will create a draft release on GitHub, which takes about a minute. The draft release can then be inspected and converted into a proper release if everything is acceptable. I prefer doing this extra human check, just to avoid anything weird happening, as any true GitHub release (but not a draft release) will trigger a PyPi release as well.

The Actions tab only allows triggering workflows from the default branch. Until then, it can be triggered from command line after installing GitHub CLI with the following command (don't forget to update the stuff between <>):

gh workflow run "Release Workflow" --ref 239_streamlined_release_dbodor -f version_level="<bumpversion_level>" -f release_branch=<branch_name>

I have created two protected test branches to check whether this action will work as intended, without messing around on main/develop. These are called test_protected_main and test_protected_develop. I have used these to create conflicts, etc, to ensure that behavior happens as intended. While testing, this filter is useful to see inspect only the current action produced here.

Specifically the workflow does the following:

Additional TODOs:

closes: #239

DaniBodor commented 5 months ago
DaniBodor commented 5 months ago

I'm requesting review from two of you. @wbaccinelli more on the actual workflow code and @psomhorst for on a higher level overview whether it's clear how to use it and whether you have any comments on the process (but both feel free to comment on both things).

Commits are made atomically so that this PR can easily be reviewed one commit at a time.

DaniBodor commented 5 months ago

For some reason, I cannot reply to this comment directly.

Is there a risk here of merging main into release_branch not resulting in a conflict/termination, but merging release_branch into main will result in a conflict/termination?

Is it possible to, in this step, check for conflicts in the other direction?

I believe that once main is merged into the release branch without conflict, the reverse cannot have a conflict. @wbaccinelli, is this correct?

Also, does this merge of main into the release branch survive? I don't think this currently matters much, because in principle we rebase everything onto develop, which has main merged into it. However, it might lead to messy git trees unnecessarily.

It only exists in the "local" repo of the workflow itself and is never pushed back to the remote, so no. Note that I am not rebasing any of these, which indeed will lead to an ugly git tree. However, there is a much larger chance of rebase conflicts than merge conflicts, which I wanted to avoid here, so decided to go with this. If you prefer we can still go with rebases, but then there will be more pressure on the developer to ensure that the release branch can be rebased, without an easy way to bypass this requirement.

psomhorst commented 5 months ago

For some reason, I cannot reply to this comment directly.

Is there a risk here of merging main into release_branch not resulting in a conflict/termination, but merging release_branch into main will result in a conflict/termination? Is it possible to, in this step, check for conflicts in the other direction?

I believe that once main is merged into the release branch without conflict, the reverse cannot have a conflict. @wbaccinelli, is this correct?

I think you are correct, but I'm not 100% sure. I could not find anything on the topic on StackOverflow and ChatGPT just makes nonsensical stuff up when asked about it. So I guess it's not an issue.

Also, does this merge of main into the release branch survive? I don't think this currently matters much, because in principle we rebase everything onto develop, which has main merged into it. However, it might lead to messy git trees unnecessarily.

It only exists in the "local" repo of the workflow itself and is never pushed back to the remote, so no. Note that I am not rebasing any of these, which indeed will lead to an ugly git tree. However, there is a much larger chance of rebase conflicts than merge conflicts, which I wanted to avoid here, so decided to go with this. If you prefer we can still go with rebases, but then there will be more pressure on the developer to ensure that the release branch can be rebased, without an easy way to bypass this requirement.

I think merging here is exactly what we want. A PR into develop should be rebased before merging, but develop should be merged into main.

psomhorst commented 5 months ago

This seems great!

I have a few comments about the process itself, and a few comments about the documentation.

I think the branching strategy is not explained well anywhere. Maybe there should be a paragraph to explain, e.g.:

## Branching strategy

`main` always contains the latest stable release version of `eitprocessing`.
`develop` contains the next release version under construction. 

When creating a new feature, one should branch from `develop`. 
When a feature is finished, a PR to pull the feature into `develop` should be 
created. After one or multiple features have been pulled into `develop`, the 
release workflow can be triggered to automatically create the new feature (minor)
release originating from `develop`. 

For bug fixes that can't wait on a feature release, one should branch from `main`. 
When the bug fix is finished, the release workflow can be triggered for a patch release
originating from the created branch. 

In principle, no releases should originate from branches other than `develop` and 
bug fix branches.

This paragraph is written assuming we don't make pre-releases, release candidates, beta's, et cetera. I think we will do this at some point (e.g. a pre-release for untested code for a specific project), but that will come later.

DaniBodor commented 5 months ago

Also, does this merge of main into the release branch survive? I don't think this currently matters much, because in principle we rebase everything onto develop, which has main merged into it. However, it might lead to messy git trees unnecessarily.

It only exists in the "local" repo of the workflow itself and is never pushed back to the remote, so no. Note that I am not rebasing any of these, which indeed will lead to an ugly git tree. However, there is a much larger chance of rebase conflicts than merge conflicts, which I wanted to avoid here, so decided to go with this. If you prefer we can still go with rebases, but then there will be more pressure on the developer to ensure that the release branch can be rebased, without an easy way to bypass this requirement.

I think merging here is exactly what we want. A PR into develop should be rebased before merging, but develop should be merged into main.

So do I interpret correctly to leave it as is?

wbaccinelli commented 5 months ago

For some reason, I cannot reply to this comment directly.

Is there a risk here of merging main into release_branch not resulting in a conflict/termination, but merging release_branch into main will result in a conflict/termination? Is it possible to, in this step, check for conflicts in the other direction?

I believe that once main is merged into the release branch without conflict, the reverse cannot have a conflict. @wbaccinelli, is this correct?

I would also say that it sounds logical. Conflicts come from lines that have been changed by both the versions, so if there are no conflicts in one direction there should be no conflict also in the other direction. I cannot be sure though, even after having consulted Lourens. Shouldn't we maybe directly test merging the release_branch in main instead of the other way around?

DaniBodor commented 5 months ago

For some reason, I cannot reply to this comment directly.

Is there a risk here of merging main into release_branch not resulting in a conflict/termination, but merging release_branch into main will result in a conflict/termination? Is it possible to, in this step, check for conflicts in the other direction?

I believe that once main is merged into the release branch without conflict, the reverse cannot have a conflict. @wbaccinelli, is this correct?

I would also say that it sounds logical. Conflicts come from lines that have been changed by both the versions, so if there are no conflicts in one direction there should be no conflict also in the other direction. I cannot be sure though, even after having consulted Lourens. Shouldn't we maybe directly test merging the release_branch in main instead of the other way around?

Fair. I guess the entire "Check for conflicts" section can be skipped and the --no-ff --no-commit / --continue process could be done directly in the next section of the workflow. I am fairly certain that would work, but don't feel like doing an additional round of extensive testing. If we go that route, would it be ok to just stick with it and fix problems in (the unlikely?) case they arise, rather than do a bunch mote testing now? @psomhorst ?

psomhorst commented 5 months ago

If we go that route, would it be ok to just stick with it and fix problems in (the unlikely?) case they arise, rather than do a bunch mote testing now? @psomhorst ?

Go for it!