-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dockerfile tests and cleanup imputation/PRS #202
Merged
Merged
Changes from all commits
Commits
Show all changes
37 commits
Select commit
Hold shift + click to select a range
56e5107
bump ubuntu versions
kachulis 57b2ca4
.
kachulis 94e135e
init dockerfile tests
kachulis 2f25449
.
kachulis 88ecaf4
.
kachulis 37a5e4a
.
kachulis 7d951d7
.
kachulis 7249556
changed dockerfiles
kachulis 80b6cda
context
kachulis 5dc93d3
context
kachulis ea9b2f8
all files
kachulis 60e746b
all files
kachulis 5bd81d7
all files
kachulis ebc7fa3
all files
kachulis 07351ee
all files
kachulis 17b39a2
list dockerfiles
kachulis c648202
**/Dockerfile
kachulis 812c9c9
bzip
kachulis d3c0c19
zlib, don't fail-fast
kachulis 303b2ac
don't fail-fast
kachulis 512ceeb
build-essential
kachulis 8d65391
test all this branch
kachulis baca1d4
.
kachulis 4f300ef
.
kachulis 352925d
fix dockers, remove Imputation
kachulis 081438f
ImputationPipeline --> PRS
kachulis b925a91
move glimpse docker tests to separate
kachulis 4574716
.
kachulis de9da7c
cache
kachulis 2c86515
setup docker buildx
kachulis 98fd217
remove caching
kachulis 0075958
set github.ref check back to refs/heads/main
kachulis 4c7ab82
ubuntu versions
kachulis c97b2e9
.
kachulis ad16ebf
PRS/ScoreBGE/requirements.txt
kachulis c53227e
ImputationPipeline --> PRS
kachulis c9806bc
cupcake comment
kachulis File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| name: Dockerfile tests | ||
| on: | ||
| push: | ||
| branches: | ||
| - main | ||
| pull_request: | ||
| paths: | ||
| - '**/Dockerfile' | ||
| schedule: | ||
| - cron: '23 2 * * 0' | ||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }} | ||
| cancel-in-progress: true | ||
| jobs: | ||
| define_changed_dockerfile_matrix: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| matrix: ${{ steps.changed-dockerfiles.outputs.all_changed_files }} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - | ||
| name: Get Changed Dockerfiles | ||
| id: changed-dockerfiles | ||
| uses: tj-actions/changed-files@v44 | ||
| with: | ||
| matrix: true | ||
| files: '**/Dockerfile' | ||
| files_ignore: '**/GlimpseImputationPipeline/glimpse_docker/Dockerfile' | ||
| - name: List dockerfiles | ||
| run: echo '${{ steps.changed-dockerfiles.outputs.all_changed_files}}' | ||
|
|
||
| define_all_dockerfile_matrix: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| matrix: ${{ steps.all-dockerfiles.outputs.files}} | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - | ||
| name: Get all Dockerfiles | ||
| id: all-dockerfiles | ||
| run: | | ||
| { | ||
| echo 'files<<EOF' | ||
| find . -name "Dockerfile" ! -path "*/GlimpseImputationPipeline/glimpse_docker/*" | jq -R . | jq -s . | ||
| echo EOF | ||
| } >> "$GITHUB_OUTPUT" | ||
| - name: List dockerfiles | ||
| run: echo '${{ steps.all-dockerfiles.outputs.files}}' | ||
|
|
||
| test_builds: | ||
| runs-on: ubuntu-latest | ||
| needs: [define_changed_dockerfile_matrix, define_all_dockerfile_matrix] | ||
| strategy: | ||
| matrix: | ||
| dockerfile: ${{ github.ref != 'refs/heads/main' && fromJSON(needs.define_changed_dockerfile_matrix.outputs.matrix) || fromJSON(needs.define_all_dockerfile_matrix.outputs.matrix) }} | ||
| fail-fast: false | ||
|
|
||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - | ||
| name: Get context | ||
| id: get-context | ||
| run: | | ||
| echo "context=$(dirname ${{ matrix.dockerfile }})" >> $GITHUB_OUTPUT | ||
| - | ||
| name: Build | ||
| uses: docker/build-push-action@v6 | ||
| with: | ||
| context: ${{ steps.get-context.outputs.context }} | ||
| file: ${{ matrix.dockerfile }} | ||
|
|
||
| test_glimpse_docker: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - | ||
| name: Check if Dockerfile has changed | ||
| id: check-if-changed | ||
| if: ${{ github.ref != 'refs/heads/main' }} | ||
| uses: tj-actions/changed-files@v44 | ||
| with: | ||
| files: '**/GlimpseImputationPipeline/glimpse_docker/Dockerfile' | ||
| - | ||
| name: Build main GLIMPSE Docker | ||
| if: ${{ github.ref == 'refs/heads/main' || steps.check-if-changed.outputs.all_changed_files_count > 0 }} | ||
| run: | | ||
| cd GlimpseImputationPipeline/glimpse_docker | ||
| ./build_base_and_extension_docker.sh -r https://github.com/odelaneau/GLIMPSE.git -t test -b master -y | ||
| - | ||
| name: Build Extract Num Sites Docker | ||
| if: ${{ github.ref == 'refs/heads/main' || steps.check-if-changed.outputs.all_changed_files_count > 0 }} | ||
| run: | | ||
| cd GlimpseImputationPipeline/glimpse_docker | ||
| ./build_extract_num_sites_from_reference_chunk_docker.sh -y -r https://github.com/michaelgatzen/GLIMPSE.git -t test -b master | ||
|
|
||
|
|
||
This file was deleted.
Oops, something went wrong.
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of testing the build of the docker image? Presumably to test any changes made in the pipeline, one would have to build the docker, upload it, and run the WDL already, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow? All of these docker tests are just to make sure they build, not to test whether the build work correctly in the pipelines. The one difference here is that we need to build the glimpse tool docker on it's own and then build the glimpse pipeline docker from that. I believe the reason for doing that was basically to not have to duplicate the glimpse tool docker in our pipeline docker, but still be able to add things to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I had a hard time understanding the use case where you'd want to update a docker image but not test it actually works or does the updated thing in your pipeline, which seems to be the case this test would help verify. For example, I'm imagining if the image needed an updated version / new tool, wouldn't you also test at the same time that the pipeline still works? Or are you decoupling this check from e.g. just bumping versions of dependencies from time to time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree testing that an updated docker actually does what you want would be even better, but I think that would require automatic build-push (easy) and associated wdl update (more complex but intriguing). For the moment I'm just thinking of this as both a check to catch obvious mistakes, and a check against external changes breaking our Dockerfiles.