-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
PR: Add workflow triggers for release candidates and fix tag normalization #20396
Conversation
@ccordoba12 I've also checked a release candidate tag on my fork on this branch as well and all looks good. |
56c6164
to
725d6be
Compare
@ccordoba12, this should be ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary for your work on this! I left some questions for you to better understand your work here.
strategy: | ||
matrix: | ||
build_type: ['Lite', 'Full'] | ||
env: | ||
UNWANTED_PACKAGES: ${{github.event_name == 'pull_request' && 'pip spyder-kernels python-slugify Rtree QDarkStyle PyNaCl psutil' || 'pip python-slugify Rtree PyNaCl psutil' }} | ||
UNWANTED_PACKAGES: ${{github.event_name == 'pull_request' && github.event.action != 'labeled' && 'pip spyder-kernels python-slugify Rtree QDarkStyle PyNaCl psutil' || 'pip python-slugify Rtree PyNaCl psutil' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This addition
github.event.action != 'labeled'
means that the PRs can't have any label after this change for it to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PRs can have any label, but we want the workflow to behave differently for different pull request actions.
Previously, this workflow triggered on the default pull_request
types: opened
, reopened
, or sychronize
. This trigger caused the workflow to build developer versions (not "release" versions). In the case of this particular line, this meant that spyder-kernels
was included in the UNWANTED_PACKAGES
variable. For "release" versions (i.e. non-PR triggers) spyder-kernels
would not be included in this variable.
With this PR, we want the behavior of the workflow to remain the same regarding release-like and non-release-like versions, but the trigger now includes the labeled
pull_request
type. This changes the logic from github.event_name == 'pull_request'
to github.event_name == 'pull_request' && github.event.action != 'labeled'
.
But what about PRs that get labeled an arbitrary label? Well, we don't have to worry about that logic on this line because the build
job will not run in this case due to line 47. github.event.action != 'labeled'
on line 47 covers all the previous trigger cases; github.event.label.name == 'build-installer-artifacts'
on line 47 then permits only the additional labeled
trigger when the label is correct, i.e. arbitrary labels given to a PR will still trigger this workflow but the jobs will be skipped.
Unfortunately, unlike the push.tag
trigger, there is no way to filter the workflow trigger on a label pattern, so it must be done at the job level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ccordoba12, did this sufficiently answer your question?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does. Sorry for the delay
You mean after this PR is merged to 5.x and master, right? Also, it seems you did a lot of testing here, so could you squash some of your commits if that was the case? |
@dalthviz, I asked for your review so that it's clear for you what you need to do in our next release and you can ask for clarifications/improvements to this workflow too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be sure I'm understanding correctly, the way the triggering of the workflows will work when doing a release is:
flowchart TD
A[Merge update dependencies PR] --> B[Commit directly to Spyder upstream 5.x to set the preX -pre0- version or update the X -0, 1, etc-]
B--> C[Trigger the installers workflows manually from the Actions page]
C--> D[Test manually the installation of the different builded installers]
D--> E{Errors where found?}
E-->|Yes| F[Submit and merge PR with fixes]
F--> B
E---->|No| G[Continue with the steps to do the release]
Right? If I'm understanding this correctly then sounds good to me 👍
Maybe having some basic set of things/tasks to test manually with the generated installers could be nice to have but I think we can open an issue to add them later to the RELEASE.md
Correct.
No problem. |
Also, thank you for working on this @mrclary I think this will greatly improve the release process reliability in terms of generated assets/installers 👍 |
That is correct.
This is a good idea. Presently, the original macOS installer only tests that Spyder.app launches without error. I think we could do the same for the original Windows installer as well as the new conda-based installers. However, when we move to 6.0(alpha?), we may be able to adapt our existing unit tests to run on a release candidate installation on CI. We can discuss the cost/benefits when we get there... |
…move leading "v". All other built repo versions should remain normalized, otherwise conda can't resolve environment for installer
Add helper environment variables for reference Update workflow logic
…Windows installers
…ner and combines two housekeeping jobs that are very fast.
build-installers may have run in the event that build-matrix was skipped (arbitrary PR label), resulting in a workflow error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary!
Description of Changes
build-installer-artifacts
Workflow Triggers
Pull requests will continue to trigger all three installer workflows with limited artifacts (Full versions only) and without code-signing or uploading artifacts.
Labeling a pull request with
build-installer-artifacts
or pushing a tag matching the patternv*pre*
will also trigger all three installer workflows, but they will run as if it were a release (Lite and Full versions, code-signing, etc.). However, workflow artifacts will not be uploaded to a release. These two trigger methods will not have any affect on a branch in which the latest commit message contains[ci skip]
(or equivalent variations).All three installer workflows can be triggered manually from the GitHub web interface or CLI and will execute workflows in the same manner as the aforementioned labeling and tagging mechanisms. This method is not affected by the presence of
[ci skip]
(or variants). However, this workflow trigger mechanism will not be available until this update is propagated to themaster
branch, i.e. after merging to5.x
and subsequent merge tomaster
.All installer workflows will continue to behave as before upon creating a release.
Normalized version
setuptools_scm
by default normalizes ourvX.X.XpreX
release candidate tags tovX.X.XrcX
, resulting in an installer namedEXPERIMENTAL-Spyder-5.4.2rc1-macOS-x86_64
instead ofEXPERIMENTAL-Spyder-5.4.2pre1-macOS-x86_64
.To work around this, we tell
setuptools_scm
not to normalize the version. This results in a versionvX.X.XpreX
, so we further remove the leadingv
, finally arriving at the desiredX.X.XpreX
.Note that the versions for
external_deps
built locally for the installer must remain normalized; otherwise conda will fail to solve the environment.