feat(ci): always test before publish #801
Conversation
There was a problem hiding this comment.
Hello! I have a couple thoughts on this one:
-
As I understand this the extra build and test run is not be necessary in anything but exceptional circumstances due to our branch protection which ensures that all commits on
mainpass CI. I would recommend we extend this PR to only run this in the case where the Build+Test workflow was not previously ran and passed for the target commit SHA. I had looked into this previously and found it not as simple to do as I might have hoped, but there is an action on the Marketplace that looks up-to-date and healthy which covers this case and is relatively easy to configure--see:
https://github.com/orgs/community/discussions/27031#discussioncomment-3254376 and https://github.com/marketplace/actions/skip-duplicate-actions
We could utilise that action to either run tests again if there isn't a passing test run on the commit we're tagging and publishing, or simply reject the publish action with status logging to note that it was rejected due to there being no passing test run for the given commit. -
Also, if I remember right I did test some scenarios where I pulled the build pushed to GHCR for the Publish step and found that the time savings was not consequential currently due to the time it takes to transfer the images down from GHCR to the actions runner server, and then push back up to Dockerhub. This one is worth double-checking if we turn on multi-platform as I think that point would be when the much longer build times will make it more worth optimising. As it is though I think we are best to just reject Publish runs on commits which don't have otherwise passing checks.
|
This PR offers definitely only a very naïve implementation. I did not want to spent too much time on the topic, this is why I did not look into more advanced options. In general I figured, I'd rather have the pipeline run twice and be sure the tests pass before release instead of requiring people to check the pipeline status of the last commit of the branch they are currently releasing from manually. My suggestion would be to use this simple approach now and implement the sophisticated one in a next step. WDYT? |
|
I'd be happier to do it now, but it will be at least a few days before I'd get to it. My reasoning: I hear ya, but the work of applying the action mentioned and eliminating the extra build and test runs for every individual image release doesn't look like much to me. However, testing Github Actions is always a bit of a drudge / problematic due to the need to be on The extra ~20 mins of time needed to know whether a release was successful or not is consequential. Obviously one can simply start the release and look back later, but realistically--at least for me--it takes up space in my brain until it's done and successful. If one starts a release and continues working without worrying about it, and something fails, I think it will be easy to miss. In summary I think the extra build+test run is adding too much friction to our new release process which I'm hoping to keep as light as possible so we all feel easy about releasing small changes. |
Fine with me, so you take over then? |
Before actually publishing the image to dockerhub, run our usual build and integration tests. This adds a simple safety net to not publish images failing tests.
Note that this simple solution builds the images twice, once for testing (images are pushed to GHCR as usual) and another time for publishing to dockerhub. Reducing the build to only once (which would be the way cleaner solution) would require quite some refactoring in the build script and some extra thought wrt multi platform builds.
https://phabricator.wikimedia.org/T378941