sopel-irc / docker-sopel

:whale: Officially Unofficialâ„¢ Docker container for Sopel, a Python IRC bot
https://hub.docker.com/r/sopelirc/sopel
12 stars 6 forks source link

Add single arch build using GHA replacing travis #42

Closed adamus1red closed 3 years ago

adamus1red commented 3 years ago

The GHA workflow pulls the version numbers to build from the SOPEL_BRANCH environment variable in the Dockerfile rather than relying on changing it in 3 different places.

Has a matrix build for the different supported python versions with Python 3.9 defaulting to be the "main" release version as the latest tag.

New build-time secrets

adamus1red commented 3 years ago

I have the proper nightly/master build in my local repo. I can push to this branch with it if you want. It uses the build-arg to specify that SOPEL_BRANCH is master.

I did not include it with this PR as I figured from your comment you wanted a clean base to start from on GHA

HumorBaby commented 3 years ago

OK, yea go ahead and include that little snippet.

Also, while you're at it: please change the Docker Hub secrets ENVs from DOCKER_USERNAME and DOCKER_PASSWORD to DOCKER_HUB_USERNAME and DOCKER_HUB_ACCESS_TOKEN, respectively. I requested @dgw to provide the needed secrets and gave him the generic instructions from https://docs.docker.com/ci-cd/github-actions. For simplicity and consistency's sake, let's stick to the names used in those instructions.

HumorBaby commented 3 years ago

For posterity:

HumorBaby: want me to push the nightly build code as well to the GHA branch or do you want it as a follow up with the multi-arch build as well? 11:01:48 <+HumorBaby> I actually just noticed a change we will need before merge anyway, so you can add the nightly build to it. 11:02:06 <+HumorBaby> Also, if you see this in time, don't bump the version just yet 11:02:33 <+HumorBaby> Make your PR just for GHA+nightly @v7.1.1 11:03:04 <+HumorBaby> It will be faster probably, since currently your PR doesn't update the README which still has references to 7.1.1 11:03:29 <+HumorBaby> A separate PR can then update everything from 7.1.1-->7.1.2 in one go 11:03:47 <+HumorBaby> AND can add a little snippet about the new `nightly` tag to the README 11:04:22 <+HumorBaby> adamus1red^ sorry if you missed this, since I have also been commenting in the GH PR itself (gonna copy-paste now)
HumorBaby commented 3 years ago

Again, for posterity:

11:21:49 <+HumorBaby> my brain was still in travel mode and so I missed this other thing: change the IMAGE secret to DOCKER_SOPEL_IMAGE_NAME please 🥺 11:22:10 <+HumorBaby> no need to act that

Also, pinging @dgw to also add this secret when adding the auth secrets mentioned above; please set to sopelirc/sopel (the name of the docker image)

adamus1red commented 3 years ago

Sample images built using this GHA https://hub.docker.com/repository/docker/adamus1red/sopel/tags?page=1&ordering=last_updated

HumorBaby commented 3 years ago

Also, it looks like you might not have staged the changes you made into your last updated commit (https://github.com/sopel-irc/docker-sopel/pull/42/commits/18039e476798b3057e844f8740679f7e5dd8fb29) where you said the requested changes were resolved. Only the messages are updated; the files are still the same.

adamus1red commented 3 years ago

@HumorBaby Added a new step which checks the variables exist since as per actions/runner#520 you can't use a secret in the job scope.

HumorBaby commented 3 years ago

for posterity:

14:37:02 <+HumorBaby> dagnabbit adamus1red, this GHA workflow condition is a real PITA
14:38:06 <+HumorBaby> honestly, I think it should go back to my original suggestion of if: ${{ github.repository_owner == 'sopel-irc' }} 
14:39:36 <+HumorBaby> anyone who forks is likely going to have their own changes (if not, they can use the base image), and in that case, they can just perma-float (append) the commit to update the GHA workflow file to their branch/tree as I desribed
14:40:55 <+HumorBaby> since there is no way to cleanly condition the 'start' of a job without at least 1 failure (when secrets are missing), forks are still going to be flooded with nightly failed workflows
14:41:45 <+HumorBaby> at that point, they will modify their actions yaml anyway, so might as well have included the owner check anyway
14:45:18 <adamus1red> HumorBaby: forks need to manually enable actions
14:45:34 <adamus1red> so it's not like all 4 forks are going to get spammed instantly
14:45:58 <+HumorBaby> lol 4 forks
14:47:54 <+HumorBaby> hmm ok I'll take your word for it for now
14:49:18 <+HumorBaby> so then lets just scrap that check all together. Everything else looked good so looks like we'll merge after this final change
adamus1red commented 3 years ago

Stripped out preflight. We can add it back later along with any other stuff we want to check before build.