search

containers / podman

Podman: A tool for managing OCI containers and pods.
https://podman.io
Apache License 2.0
22.4k stars 2.31k forks source link

[skip-ci] GHA: Send release notification mail #22988

Closed cevich closed 3 weeks ago

cevich commented 3 weeks ago

Rather than manually crafting what ends up being nearly identical release e-mails, do it automatically whenever a release is created.

Note: At the time of this commit, there is a possible race condition with the mac-pkg.yml workflow, since it runs in parallel. It could fail, or fail to complete prior to the e-mail content being generated. This should be unlikely, if release-artifacts.yml goes through and compiles every artifact, but it's not guaranteed.

Does this PR introduce a user-facing change?

None
openshift-ci[bot] commented 3 weeks ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cevich Once this PR has been reviewed and has the lgtm label, please assign lsm5 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/containers/podman/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cevich commented 3 weeks ago

@ashley-cui @mheon this is ready for review. I've tested it as best as I can over in my fork, here's a recent run (using dummy binaries). Obviously sending the mail failed b/c I didn't setup any secrets on my fork. However that action-send-mail thing is well used elsewhere so I'm pretty confident it will work.

Edit: This is GHA, so manual merge is needed.

packit-as-a-service[bot] commented 3 weeks ago

Cockpit tests failed for commit b40f5d4231d301b07d56316e7a01d254de0db85d. @martinpitt, @jelly, @mvollmer please check.

cevich commented 3 weeks ago

I think we have some debug statements?

Maybe those step names are misleading. Would it help if I renamed them to "Provide ~<FOO~> contents"?

mheon commented 3 weeks ago

LGTM

ashley-cui commented 3 weeks ago

Maybe those step names are misleading. Would it help if I renamed them to "Provide ~<FOO~> contents"?

I think that would be better! But mostly a nit, either way, LGTM

cevich commented 3 weeks ago

I think that would be better! But mostly a nit, either way, LGTM

Thanks for your feedback, I think I'll make the change. Afterall, there's no CI to wait for :stuck_out_tongue_winking_eye:

cevich commented 3 weeks ago

Force-push: Fixes based on feedback. Also added a minor list-indentation fix.

Here's a manual test of the https://github.com/cevich/podman/actions/runs/9500638269/job/26184173832#step:3:25.

cevich commented 3 weeks ago

Here's a manual test showing dummy release mail content.

I believe this is ready for final review and manual merging.

ashley-cui commented 3 weeks ago

One more question: It looks like in your test run, if the build workflow ends with no new artifacts need to be built, the email is still sent. Does this mean that we could have duplicate emails?

cevich commented 3 weeks ago

Does this mean that we could have duplicate emails?

Good eyes, yes I think that would be the case. Let me see if I can fix that.

ashley-cui commented 3 weeks ago

@ashley-cui the other thing I noticed during testing: if any of the builds fail, for example due to an incompatible Makefile, everything gets blocked up. Other than fixing the Makefile or workflow, the only other option is to manually build and upload the offending release file, then re-run the job (wherein, that build step will get skipped). I'm choosing to stick my head in the sand about this, but I'm happy to file an issue if you think it's something worthy of tracking.

@cevich I think it should be fine, worst case we realize the email didn't go out and we manually send it. But for duplicates, I wish there was a way to track if we've sent an email before. Unfortunately I'm not entirely sure if having this the email as a separate GHA from artifacts would have any benefits other than a dedicated re-run email only button. I suppose that doesn't solve duplicates, unless we do a release-only trigger and not have it depend on artifacts.

cevich commented 3 weeks ago

woops, sorry got my tabs mixed up :blush:

worst case we realize the email didn't go out and we manually send it.

SGTM

cevich commented 3 weeks ago

Manual test showing the notification job is skipped when nothing was built and/or the upload was unsuccessful (e.g. --clobber is only specified for shasums) in "Upload to Release"

cevich commented 3 weeks ago

Ugg, woops, spotted a bug...

cevich commented 3 weeks ago

Once again for the fifth time, now I think this is actually ready for a final look and possible manual merging :grin:

ashley-cui commented 3 weeks ago

LGTM, I'm going to push the button for manual merging.