search

KhronosGroup / DockerContainers

Docker container specifications which package dependencies for building Khronos documentation and software
Apache License 2.0
7 stars 10 forks source link

Migrate from Azure to GitHub Actions and reuse scripts #18

Closed MarijnS95 closed 2 years ago

MarijnS95 commented 2 years ago

Azure is less convenient to use as it runs off-site, and redefining image names and versions is error prone when the scripts in this repo can be called instead, to smoke-test them too.

MarijnS95 commented 2 years ago

GitHub CI won't run before the first configuration is merged to master. You can see the run on my repo: https://github.com/MarijnS95/DockerContainers/runs/7147521709?check_suite_focus=true#step:3:5.

oddhack commented 2 years ago

I may have missed something, but ISTM pushing an image from a CI job is going to require a secret key to authenticate to dockerhub.

oddhack commented 2 years ago

Also I am not clear on whether the other PRs you have open making changes in the Azure setup are obsoleted by this PR? If so seems better to work on getting this ready to go.

Regarding dockerhub organization membership, we apparently are paying for, and using, exactly 7 seats in the khronosgroup organization. I opened a gitlab tracker for James to ask about either alternative methods of allowing you to push an image, or paying Dockerhub a bit more to get another seat. In the meantime there's a khronosgroup/rust repository but you still can't populate it :-(

MarijnS95 commented 2 years ago

I may have missed something, but ISTM pushing an image from a CI job is going to require a secret key to authenticate to dockerhub.

We discussed in https://github.com/KhronosGroup/DockerContainers/pull/15#issuecomment-1155534815 to drop the push part from CI entirely, so it has been removed in that PR. Hence not carried over into here either.

This CI step is only build-testing the images, fwiw - doesn't need a token.

Also I am not clear on whether the other PRs you have open making changes in the Azure setup are obsoleted by this PR? If so seems better to work on getting this ready to go.

I don't really mind: part of the changes are scripts, part are Azure. Whatever lands first, I'll fix conflicts to match. If this PR makes it in first, I can simply drop all the Azure changes in the other PRs and be left with just script changes.

Regarding dockerhub organization membership, we apparently are paying for, and using, exactly 7 seats in the khronosgroup organization. I opened a gitlab tracker for James to ask about either alternative methods of allowing you to push an image, or paying Dockerhub a bit more to get another seat.

I don't per-se need push access to the repo, but may then have to rely on you to push the original image and/or any updates we decide to do. These should have a version bump inside this repo - through a PR - anyway.

This issue may be a nice reason to set up a secret key and "push" job in GitHub CI after all? That way there's no dependency on anyone after a PR is merged, but it means we'd also be re-pushing the same versioned tags with "updated" images (e.g. apt may be pulling in newer packages). If that's an issue we'd need to make it smarter (if tag exists upstream, don't update it to point to a newer image; only do that for latest).

In the meantime there's a khronosgroup/rust repository but you still can't populate it :-(

Just making sure: I'm fine having it under khronosgroup/docker-images, unless we migrate all images to their own khronosgroup/<image>, to not have any additional scripting complexity (or extra arguments that need to be manually passed). See also https://github.com/KhronosGroup/DockerContainers/pull/13#issuecomment-1172524046 and #17.

oddhack commented 2 years ago

Just making sure: I'm fine having it under khronosgroup/docker-images, unless we migrate all images to their own khronosgroup/<image>, to not have any additional scripting complexity (or extra arguments that need to be manually passed).

It comes down to whether @rpavlik wants to migrate the OpenXR images to their own dockerhub repositories, then. I will go along if so, but am content as things are now.

oddhack commented 2 years ago

GitHub CI won't run before the first configuration is merged to master. You can see the run on my repo: https://github.com/MarijnS95/DockerContainers/runs/7147521709?check_suite_focus=true#step:3:5.

Supposedly there is a way of enabling workflows for you, per https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks, but I don't see the 'workflows awaiting approval' dialog that page mentions so am unclear what's going on. Is the restriction that there must be CI in the default branch before CI will run anywhere documented? I'm happy to merge this if @rpavlik is also OK with it.

MarijnS95 commented 2 years ago

Supposedly there is a way of enabling workflows for you, per https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks, but I don't see the 'workflows awaiting approval' dialog that page mentions so am unclear what's going on.

@oddhack workflow approval was a way for GitHub to push abusive behaviour prevention of the runners themselves onto the maintainers. They'd receive a bunch of spam PRs that changed the CI to crypto miners, that'd run a job for hours to no end.

Is the restriction that there must be CI in the default branch before CI will run anywhere documented?

I think that that's the case, but will have to search for it. It should be enabled by default in the repo settings though. (This is also why there's no "approve workflow run" button here yet)

MarijnS95 commented 2 years ago

Part of this is my mistake: I only added a push: trigger, so workflows won't run on PRs (unless a PR is created from a branch that was pushed to this repo, not a fork).

I guess the pull_request: trigger is only registered from the default branch: even if I add it to my branch it doesn't magically start running within this repository until that code reaches master.