aiidateam / aiida-core

The official repository for the AiiDA code
https://aiida-core.readthedocs.io
Other
411 stars 184 forks source link

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

Closed danielhollas closed 3 weeks ago

danielhollas commented 3 weeks ago

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[bot] commented 3 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6451/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | Coverage Δ | | |---|---|---| | [presto](https://app.codecov.io/gh/aiidateam/aiida-core/pull/6451/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam) | `73.19% <ø> (?)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=aiidateam#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielhollas commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

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 commented 3 weeks ago

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

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 commented 3 weeks ago

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 https://github.com/aiidateam/aiida-core/pull/6451#discussion_r1629236824 :

@@ -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 https://github.com/aiidateam/aiida-core/pull/6451#discussion_r1629236824, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACIY64JIZXERBQNYRUU6BCLZGA2M7AVCNFSM6AAAAABI2NO6AGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCMBRGU3DKNZQGU . You are receiving this because you were mentioned.Message ID: @.***>