CI: Tweak Cirrus build filter to allow tag pushes #699
Conversation
We were trying to filter for a certain PR label before, but inadvertently checked the wrong env var, based on interpreting some guidance we got from Cirrus team. In researching what it should be instead, it occurred to me we can indeed use the `CIRRUS_TAG` env var to check for *any tag push*, which should actually do pretty well at filtering for releases after all, since releases are the only times we really push tags.
|
While overall this does sound really good, and I'm happy to read the docs as needed, one concern I do have. When a new tag is published, which triggers cirrus to run, will it just run off the master copy? Since at that point the version won't be whatever the tag is within the Just something we might want to be aware of during a test. Actually confirm the exact version string as reported by Pulsar. But beyond that, thanks a ton for addressing this, as it definitely needs to be |
Yeah, that's how I read it, Cirrus takes the repo content overall from what triggers the build, but due to our setting on the dashboard it will use whatever CI config is on
I can test now, and prove that the current And/or I can temporarily toggle the dashboard setting and then tag, to try and test this PR's change working and actually running tasks. |
|
Oops, I had set the dashboard wrong, it's doing its task filtering based on Incidentally, that means testing worked great to demonstrate this PR. See these runs: https://cirrus-ci.com/github/pulsar-edit/pulsar/CI-tweak-Cirrus-build-filters
Three "skipped" builds for branch pushes, and one running build for the tag push for tag EDIT to add: I need to go and properly lock down the Cirrus dashboard setting like I thought I had done already... Done. ALSO: The rolling upload script was smart enough not to upload a Rolling! Very cool! https://cirrus-ci.com/task/6004538386153472?logs=rolling_upload#L21 |
There was a problem hiding this comment.
Well glad you were able to test this so quickly even if it came from a slight misconfiguration lol.
But super happy to see Cirrus is operating in my hoped way, and knowing that this looks great to me! And looks like this may be one of our last CI centered PRs needed for the next little bit, which is nice to see.
Super rad work on this one
|
Alright, thank you for the review, merging! |
Short Summary / Purpose
This is a really long PR body for a very short change. This is trying to align our Cirrus build filters to do what we want them to do -- limit builds only to cron (for Rolling) builds, along with one build per release during our Regular release process.
Identify the Bug
See this comment thread: #682 (review)
To summarize: the build filtering logic in
.cirrus.ymlat the moment seems to be slightly different from what was intended.(This filtering logic is the main mechanism we use to restrict what circumstances Cirrus tasks run in, now that the free Cirrus usage has some significant upper limits placed on it per month.)
Description of the Change
In the
.cirrus.ymlCI config definition file, the part where we useonly_if:to filter what conditions the tasks will run in... Check the Cirrus env varCIRRUS_TAGis non-empty, meaning the current build was triggered by a tag being pushed to this repo.This, along with the existing cron build filter, would mean we only build for "cron builds or tag pushes." Which I think is about right.
Alternate Designs
See this whole thread for discussion: #682 (comment)
(This is a long section about various alternatives that I don't think are worth it, given the approach I actually went with seems at minimum just slightly better, IMO. Feel free to skip this section, if you want!)
CIRRUS_PR_LABELS == "regular_release"was the approach proposed just before I stumbled upon this one. But I feel like having to set a PR label is a little more cumbersome than having Cirrus just automatically build for when the tag is pushed. I think that would also require conscious timing/order of operations -- setting the PR label before publishing the PR or ideally just before pushing the "change version to 1.X.0 (no -dev)" commit. Otherwise (I think), the build's tasks will skip and no binaries will be produced... Furthermore, there is the risk of multiple builds happening per release and using double credits unintentionally/undesirably, since we usually do push a "re-add the-devto the version string" commit before merging the version bump PR's...As another option, I believe
required_pr_labels: regular_releasewould wait for the label and only then trigger the tasks, which fixes the "requires some timing/order of operations" concern. But still requires a PR label, whereas this PR currently just needs a tag to be pushed, adding no manual work to the release flow. And the "PR label" filtering approach still risks a wasteful second build when doing the "remove-devfrom the version string" commit we usually do at the end of the version bump PR's.We also have a bunch of other Cirrus env vars we may be able to check, but I stopped looking into them, since this PR's approach seemed to be a better match than any of those. But for example,
CIRRUS_RELEASEhappens if you create a tag during the creation of a GitHub Release, which if we make the tag manually, as we usually do, won't work. (But side note, if we do start making the tag from, say the GitHub Releases web UI, the approach I took in this PR is still valid. And could enable us to automatically upload the binaries to the right raft release here at core repo!)We could also look at
CIRRUS_PR_TITLEorCIRRUS_PR_BODYand see if they include "Release" or something, but searching in arbitrary text is imprecise, and might risk running release-specific stuff where there wasn't actually an intent to do so, and risk wasting credits.See: https://cirrus-ci.org/guide/writing-tasks/#environment-variables for all the various Cirrus env vars we could work with...
Possible Drawbacks
Building on every tag push means, if someone randomly pushed a tag without specifically intending to run a release, we would waste credits, and depending on how much automation is set up down the line, might automatically attempt to upload some binaries to a GitHub release. We should do a PSA for those with write access to please not push any random tags, but then again, people already don't do that, so... Also, we costed out our credit use very conservatively to stay within our limits. Chances are we could easily handle one or two, maybe even several branch pushes and still make it through the month in terms of credits.
But yeah, we should avoid pushing random tags if we go with this simple "on tag pushed" approach.
Verification Process
Admittedly not tested, but this aligns with the docs at https://cirrus-ci.org/guide/writing-tasks/#environment-variables.
It's also used in a real-world example Cirrus team pointed us to: https://github.com/cirruslabs/macos-image-templates/blob/59624221fec15db250bfcad9e68d0e8931d2ccdc/.cirrus.yml#L33 (From this discussion where @confused-Techie got some valuable info on how we could start limiting the builds: cirruslabs/cirrus-ci-docs#1223 (comment))
We will want to test this down the line, but testing it requires this config change being present on a tag that's pushed, so... I suppose I can push a tag off this branch to test it??? (And then delete said tag.)
TODO.
Release Notes
N/A
(Do we do Changelog entries for CI stuff???? I don't think we do. I dunno.)