[devops] Build and test Docker image on PRs from forks

[danielhollas]

The strategy here is simple: Build and test Docker images in a single workflow, without pushing to ghcr.io. That way we don't need access to GITHUB_TOKEN.

The downside is that one cannot download the image for local debugging. But if that is needed, I guess one can either build locally, or re-open the PR from origin.

I think @unkcpz had something else in mind, allowing forks access to secrets via pull_request_target event. I am not sure if that is safe / desirable, let's discuss that in #6446. This PR should work in the meantime.

Closes #6449
Accompanies #6446

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.86%. Comparing base (ef60b66) to head (40a6b38).
Report is 112 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6451      +/-   ##
==========================================
+ Coverage   77.51%   77.86%   +0.36%     
==========================================
  Files         560      562       +2     
  Lines       41444    41794     +350     
==========================================
+ Hits        32120    32539     +419     
+ Misses       9324     9255      -69     

Flag	Coverage Δ
presto	73.19% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danielhollas]

This is now ready. The one downside is that the tests for different images are run sequentially and not in parallel, but they are quite fast anyway and the whole workflow takes around 6-8 minutes.

[sphuber]

Thanks @danielhollas . This may be a naive question, but why can't we just take the existing docker.yml, make it run on push to any remote (main as well as forks) and then simply skip the publish step on forks. This would be the desired behavior, right? And that would prevent having to duplicate some of the workflow logic in a separate workflow file

[danielhollas]

@sphuber it's a good question. We can't use it, at least in the current form, since the first publish to ghcr.io happens already during the build step. The publish workflow actually just adds tags to an image that's already there.

We could slice and dice things differently, but honestly the new workflow is quite simple so I am not worried about the duplication. The most complicated thing is the build step, and that would have to be different anyways. The rest of duplication is not much worse than duplication that's happening in all the other workflows as well, imo.

Happy to answer more questions, but I would like to avoid yet another refactor. :-D

[sphuber]

Thanks for the explanation @danielhollas . Ok, let's not go into that for now but keep it in the back of our heads for the future to see if we can unify them. Don't want to put that on your plate just before the holidays

[unkcpz]

@danielhollas thanks! This strategy looks reasonable, and I'd say it has the same running order as my old implementation 😜

I think @unkcpz had something else in mind, allowing forks access to secrets via pull_request_target event.

It is not good to use pull_request_target actually, it expose the GITHUB_TOKEN to forked repo and need to be very careful around setting the triggering target. If this can be achieved with such simple change, using pull_request_target is overkill.

[danielhollas]

Hmm, now I am realizing, that I am not sure whether this change will not break the full build. 🤔

[unkcpz]

I remember you use env.hcl for aiidalab-docker-stack, did you use it here as well?

[unkcpz]

During test, I find it a bit annoying that need to careful about whether put / (same as : in front of the TAG). I think we can always force the REGISTRY is set, either to docker.io or to ghcr.io, then use:

 ${REGISTRY:-}/${AIIDA_CORE_WITH_SERVICES_IMAGE:-aiidateam/aiida-core-with-services}:${TAG:-}

instead of:

 ${REGISTRY:-}${AIIDA_CORE_WITH_SERVICES_IMAGE:-aiidateam/aiida-core-with-services}${TAG:-}

[sphuber]

Hmm, now I am realizing, that I am not sure whether this change will not break the full build. 🤔

Not sure if it is because of this, but the build did just fail on main: https://github.com/aiidateam/aiida-core/actions/runs/9398592676/job/25884242952

[unkcpz]

Seems it is, it tries push to docker.io as default registry.

[danielhollas]

Yeah, sorry about that, will open a PR shortly. Dne čt 6. 6. 2024 12:26 uživatel Jusong Yu ***@***.***> napsal:

…

***@***.**** commented on this pull request. ------------------------------ In .docker/docker-bake.hcl <#6451 (comment)> : > @@ -13,7 +13,6 @@ variable "ORGANIZATION" { } variable "REGISTRY" { - default = "ghcr.io/" Seems it is, it tries push to docker.io as default registry. — Reply to this email directly, view it on GitHub <#6451 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACIY64JIZXERBQNYRUU6BCLZGA2M7AVCNFSM6AAAAABI2NO6AGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBRGU3DKNZQGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>