Staged images 1 GitHub workflows #727
Conversation
moving inline with changes to the fork. * Adds latest and v1 tags to master * registry is the whole url now.
| run: | | ||
| sudo curl -L https://github.com/hadolint/hadolint/releases/download/v${{ env.HADOLINT_VERSION }}/hadolint-Linux-x86_64 --output hadolint | ||
| sudo chmod +x hadolint | ||
| ./hadolint images/${{ inputs.directory }}/Dockerfile --no-fail |
There was a problem hiding this comment.
I'm not opposed to failing on lint
| echo "BASE_IMAGE=${{ inputs.base-image }}" >> $GITHUB_ENV | ||
| fi | ||
|
|
||
| - name: Set FROM and as in Docerfile |
| # Scan image for vulnerabilities | ||
| - name: Aqua Security Trivy image scan | ||
| run: | | ||
| curl -sfL https://raw.githubusercontent.com/aquasecurity/trivy/main/contrib/install.sh | sh -s -- -b /usr/local/bin ${{ env.TRIVY_VERSION }} |
There was a problem hiding this comment.
would love to see a checksum since we are pulling this install script from the internet. Otherwise I think it's fine.
There was a problem hiding this comment.
I don't think there's one offered upstream :( we can pull the script, look at it (confirm it's fine in quality), compute the sha, then use that as our reference. Then compute the SHA on retrieval from the internet and compare.
| with: | ||
| image: "base" | ||
| directory: "base" | ||
| base-image: "quay.io/jupyter/datascience-notebook:2024-06-17" |
There was a problem hiding this comment.
I assume if we want to update the base image, we have to do it here as well as the Base dockerfile
There was a problem hiding this comment.
We would actually only do it here.
It's set this way because the From *** as *** cannot take any variables,
so the code is set to inject the line dynamically from the variables found in the docker.yaml file.
|
@mathis-marcotte to provide a review before merge. |
| needs: [vars] | ||
| uses: ./.github/workflows/docker-pull-test.yaml | ||
| with: | ||
| image: "jupyterlab-cpu" |
There was a problem hiding this comment.
The only difference between all these jobs is the "image" variable (from what I can tell).
Is there any way to reduce the code duplication here with the use of a matrix for the list of images? We currently have it defined here in the current workflow https://github.com/StatCan/aaw-kubeflow-containers/blob/master/.github/workflows/build_push.yaml#L72
| REGISTRY_USERNAME: ${{ secrets.REGISTRY_USERNAME }} | ||
| REGISTRY_PASSWORD: ${{ secrets.REGISTRY_PASSWORD }} | ||
|
|
||
| # Test the Images |
There was a problem hiding this comment.
Any way that this section and the "docker-nightly.yaml" workflow can share code?
They both seem to just run "docker-pull-test" on the list of our images. Although i do see that the "needs" section here is more than just "vars".
This is making me thing that in this current version of the workflow file, these tests can be run as soon as the build step is completed for that particular image. In a scenario where code would be shared between this and the nightly build workflow, that might change the behavior and the tests would need to be ran after all the builds are complete, which sounds like would be a longer workflow action in the end.
Anyways, not saying this absolutely needs to change, just wanted to get you thinking if we could limit code duplication between files.
There was a problem hiding this comment.
I can't think of a way to solve the trade off between reducing code duplication and running the tests as the image completes.
The tests all running at the end shouldn't effect the overall workflow time much, as they are roughly all the same time as each other. But will increase the time a developer has to wait for the results of the shorter build images to be tested.
Maybe that would be worth the trade off though?
Part one of the migration to staged builds.
This part includes all changes to the github workflows, and some changes to the makefile to support the new functionality.
A diagram of the relationships is shown below
