Skip to content

Add GH action to periodically build docker images#455

Merged
lesteve merged 16 commits into
dask:masterfrom
andersy005:ci-image-periodic-builds
Sep 29, 2020
Merged

Add GH action to periodically build docker images#455
lesteve merged 16 commits into
dask:masterfrom
andersy005:ci-image-periodic-builds

Conversation

@andersy005
Copy link
Copy Markdown
Member

Fixes #433 and amends #432

For this action to work properly, we need to set both the DOCKER_USERNAME, DOCKER_PASSWORD secrets in the repo settings. However, I currently don't have admin rights on this repo.

@andersy005 andersy005 changed the title Add GH action to periodically build images Add GH action to periodically build docker images Jul 27, 2020
Copy link
Copy Markdown
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great.

I've created us a bot user on Docker hub called daskbot and populated the secrets in GitHub.

Comment thread .github/workflows/build-docker-images.yaml Outdated
Copy link
Copy Markdown
Member

@lesteve lesteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @andersy005 for this PR! And thanks @jacobtomlinson for doing the missing bits (I wasn't sure whether there was already a specific user or people were using their user credentials for this ...).

A few questions:

  • is there a way to test this prior to merge?
  • once it is merged is there a way to trigger the cron GH action manually or should we just wait until the next scheduled time?

Comment thread README.rst Outdated
@jacobtomlinson
Copy link
Copy Markdown
Member

I wasn't sure whether there was already a specific user or people were using their user credentials for this

I think we have a range of credentials being used in various places. I'm slowly trying to standardise things to try and drop reliance on specific maintainer accounts.

once it is merged is there a way to trigger the cron GH action manually or should we just wait until the next scheduled time?

It may be good to add push: to the list of triggers so that this runs on merge to master too.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jul 28, 2020

I think we have a bit of a weird setup and I am not sure how to simplify it and how to make it play well with GH actions.

  • the docker images are only used in our CI (those are the docker-compose setups to have toy clusters for Slurm, PBS, etc ...). Originally we were building the Docker images inside the CI (so on each commit) which was a bit slow (especially in a PR) and also it is easier for running the test locally to do docker pull rather than building the images locally.
  • on a push to the master branch you probably want to generate the Docker files and push them to DockerHub before running the CI. AFAIU having separate ci.yml and build-docker-images.yaml does not make that straightforward so maybe we want a single .yml?
  • even then you can not really test any change to the toy clusters docker-compose setup (i.e. ci/slurm, ci/pbs) in a PR but I think that's fine because they change very rarely ...

@andersy005
Copy link
Copy Markdown
Member Author

on a push to the master branch you probably want to generate the Docker files and push them to DockerHub before running the CI. AFAIU having separate ci.yml and build-docker-images.yaml does not make that straightforward so maybe we want a single .yml?

It's my understanding that

  • decoupling the docker image build from the CI has a few advantages including having different event triggers for jobs defined in ci.yml and build-docker-images.yaml. Combining ci.yml and build-docker-images.yaml into a single workflow will likely make the process more complicated since the GitHub action event triggers will be the same across the entire workflow.
  • since we will be building these docker images on a daily basis, running the CI with the latest image (even if it's a day or a few hours old) won't cause any problem,

@andersy005
Copy link
Copy Markdown
Member Author

Checking in here. I am curious whether there are any additional issues/tasks this PR needs to address before getting merged?

@guillaumeeb
Copy link
Copy Markdown
Member

Sorry @andersy005 for the delay, I did not follow this change, and it's a bit complicated to get into...

I'll wait one more week for @lesteve, and then I'll try to look closer.

At one point, we might want to make you a maintainer of this repo too.

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Sep 29, 2020

Sorry I didn't have much time to devote to dask-jobqueue unfortunately.

I am going to merge this one and see what happens.

At one point, we might want to make you a maintainer of this repo too.

100% agreed, I sent you an invitation @andersy005!

@lesteve lesteve merged commit a4a218a into dask:master Sep 29, 2020
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Sep 29, 2020

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Oct 1, 2020

Actually I noticed that the images don't get pushed for scheduled events, e.g. https://github.com/dask/dask-jobqueue/runs/1190757827.

I am guessing the condition needs to be changed:
if: github.ref == 'refs/heads/master' && github.event_name == 'push'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Periodically build images for CI

4 participants