CI Refactor#133
Merged
Merged
Conversation
MarkKoz
added
type: feature
MarkKoz
force-pushed
the
ci-refactor
branch
3 times, most recently
from
c6bca44 to
5e53fca
Compare
load: true was already creating a tarball, but it was getting immediately loaded. Since no other Docker builds run in this job, it's useless to load it. The action can still be leveraged to create the tarball instead of manually invoking `docker save`.
Make the artefact and file names identical to simplify things. The artefact name doesn't have to be unique anyway since it can only be downloaded by the same workflow run.
MarkKoz
force-pushed
the
ci-refactor
branch
2 times, most recently
from
6c9252e to
238af38
Compare
Unlike the cache action, the build-push action's GHA cache feature seems to only do an exact comparison for the scope. Thus, new commits lead to cache misses.
MarkKoz
force-pushed
the
ci-refactor
branch
2 times, most recently
from
921ebe6 to
139f325
Compare
Remove the dependency on the container so the lint job can run in parallel with the build job. More time has to be spent installing Python dependencies, but this is made up for by not having to download and load the image artefact in addition to not having to wait for the build job.
The step was running even if the pre-commit hooks step never ran.
Use docker-compose run instead of docker-compose up. This is more appropriate since the container is only needed for one command. The latter was actually starting the whole snekbox server. Furthermore, the former has the --rm option to remove the container when the command finishes. As an extra precaution, use docker-compose down in the self-hosted runner to also remove images, volumes, networks, and any other containers that were somehow missed. Removing images will also prevent the disk usage from building up. This is not necessary for the GH-hosted runner since a new VM is used for each run.
MarkKoz
force-pushed
the
ci-refactor
branch
5 times, most recently
from
12a5767 to
016b83d
Compare
lemonsaurus
approved these changes
Feb 13, 2022
Contributor
lemonsaurus
left a comment
lemonsaurus
left a comment
There was a problem hiding this comment.
Honestly, this is really really good. I learned quite a bit from reading this PR. This is industry professional level CI, and it's well documented, clean, and very clever. You're using some nice modern features to keep things lean and dry.
Exceptional work. 🏆
Comment on lines
+68
to
+73
| # Output linting errors in the format GitHub Actions recognises for | ||
| # annotations. | ||
| - name: Run flake8 | ||
| run: >- | ||
| flake8 --format "::error | ||
| file=%(path)s,line=%(row)d,col=%(col)d::[flake8] %(code)s: %(text)s" |
Contributor
There was a problem hiding this comment.
oooh, this is fancy. I like it. 👍🏼
Contributor
Author
There was a problem hiding this comment.
We already do this in other projects, actually. I think I just copied it over from one if it wasn't already in the old snekbox CI.
Comment on lines
+18
to
+19
| artefact: ${{ needs.build.outputs.artefact }} | ||
| tag: ${{ needs.build.outputs.tag }} |
Contributor
There was a problem hiding this comment.
holy shit I had no idea this was legal syntax but it's so cool.
There isn't enough "meat" to warrant their use.
The latter is consistent with GitHub Action's documentation.
There are no pipes the in script, so the presence of -o pipefail may confuse readers.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The workflow is broken up into separate reusable workflows. No more of the monolithic workflow file! Another consequence is that the lint workflow can run concurrently with the build workflow, since linting doesn't depend on the build.
The build uploads a tarball of the Docker image as an artefact so that other workflows can download the artefact and start a container. For tests, the coverage results from each OS in the matrix are combined into one and finally published.
I think the overall duration of the new workflow shouldn't be much longer than the current workflow, especially due to being able to run linting in parallel despite the additional dependency install taking more time.