search

aiidalab / aiidalab-qe

AiiDAlab App for Quantum ESPRESSO
https://aiidalab-qe.readthedocs.io/
MIT License
9 stars 14 forks source link

RWIP: docker workflow refactor GHA #730

Open unkcpz opened 1 month ago

unkcpz commented 1 month ago

An analogous refactor of https://github.com/aiidalab/aiidalab-docker-stack/pull/439 by @danielhollas

Since this repo only build one image, I slightly change the extract-image-names.sh to extract-image-name.sh to get only single image name.

unkcpz commented 1 month ago

Not uploading image as artifacts.

As you mentioned as a side effect in https://github.com/aiidateam/aiida-core/pull/6388, this becomes quite annoying for qeapp, since the test on the built image help a lot with speed up the integration test and we want test happened for the PR from forked repo.

Here I'll probably do a workaround by both upload as artifacts and upload to ghcr.io if it is from the repo. The test will use the image from the artifact for the moment, and the image publish will use the image from ghcr.io

Maybe there will be potential security issue if we allow PR to push image to registry. Let's image a case that someone maliciously create an image that has racist remarks included, it will be able to be download under ghcr.io/aiidalab/.

But I think we already some restrictions that for the first contribution PR it has to be approved to be run with CI. So it may not be a big issue. Then we can instead of using github token but using the stored account and secret to make it able to upload to the ghcr.io? What you think? @danielhollas

danielhollas commented 1 month ago

Hmm, that's tricky. I understand that indeed for integration tests you'd want to use the build image. But uploading to artifacts slows things down and makes everything much more complex.

I am not sure if there is a good solution currently, I would probably simply suggest to push to origin. :-) Pushes from forks should still work, but will skip the image build and integration tests.

unkcpz commented 1 month ago

I would probably simply suggest to push to origin.

For aiidalab-docker-stack and aiida-core I think this may not be a big issue, since only core developers manage the docker images. But for the qeapp, the integration test is the test that we relied on. We used it to spot some critical issues for the QeApp quite many times. This repo start to getting contributions from > 5 people, I also tried to push to always open PR from forked repo and got a small complain from @edan-bainglass that branches of this repo is quite massed.

unkcpz commented 1 month ago

Okay, I think I can bind the build and test as what was happened before. Then I only separate the upload out and upload to ghcr.io. (updated) Not possible, the build and upload to ghcr.io is bound by back-action. Now I remember why I initially didn't use the back-action. I want the build and the upload to ghcr.io happened in different steps/jobs. But I think we can find the way, probably upload to ghcr.io from forked repo is not a big problem. I'll see how to confine the permission of the token.

danielhollas commented 1 month ago

Yeah, all valid points. I don't think I have anything more clever to say, it's a tradeoff.