Skip to content
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

integration of test for dockerfile update #754

Merged
merged 77 commits into from
Jul 18, 2021
Merged

Conversation

bam241
Copy link
Member

@bam241 bam241 commented Jul 12, 2021

duplicating test for docker file update.

@bam241
Copy link
Member Author

bam241 commented Jul 12, 2021

also added a contiditon push based on PR status

@bam241
Copy link
Member Author

bam241 commented Jul 12, 2021

@shimwell do you know if my implementation will use cashed img ?

@shimwell
Copy link
Member

shimwell commented Jul 12, 2021

@shimwell do you know if my implementation will use cashed img ?

I can see 12 jobs all running in parallel as the first stage. I don't think these will make use of cached images.

Perhaps the needs argument can be used in a few places to make sure the base images have been built first

I think the test_docker section will make use of previously build cached images that were build in the first section, but it should all appear in the logs

build-args: |
UBUNTU_VERSION=${{ matrix.ubuntu_versions }}
tags: ghcr.io/svalinn/dagmc-ci-ubuntu-${{ matrix.ubuntu_versions }}
tags: ghcr.io/svalinn/test_dagmc-ci-ubuntu-${{ matrix.ubuntu_versions }}
Copy link
Member

Choose a reason for hiding this comment

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

This will need to include some other unique string (sha?) to avoid two simultaneous PR's from overwriting each other


- name: Environment setup
run: |
if [ "${{ matrix.isMerged }}" == "true" ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I don't understand how the isMerged works, but this seems like the logic is reversed. Don't we want the sha for PRs and without a sha for merged?

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Oops

ARG UBUNTU_VERSION=18.04
FROM ghcr.io/svalinn/dagmc-ci-ubuntu-${UBUNTU_VERSION}
ARG UBUNTU_VERSION=18.04ARG
ROOT_NAME=dagmc
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ROOT_NAME=dagmc
ARG ROOT_NAME=dagmc

@bam241
Copy link
Member Author

bam241 commented Jul 15, 2021

closing and reopening from upstream

@bam241 bam241 closed this Jul 15, 2021
Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

This should fix the most recent error

CI/Dockerfile_3_moab Outdated Show resolved Hide resolved
Co-authored-by: Paul Wilson <paul.wilson@wisc.edu>
@bam241
Copy link
Member Author

bam241 commented Jul 17, 2021

@gonuke thx for the suggestion ! it seems tone working now !

One need to make sure the retagging is working well, then I disable retag on personal branch and this should be good to go.

@bam241
Copy link
Member Author

bam241 commented Jul 17, 2021

not that I rearranged @shimwell slicing to avoid uploading image we don't use or don't need to exchange between jobs...

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Another typo?

.github/workflows/docker_publish.yml Outdated Show resolved Hide resolved
@gonuke
Copy link
Member

gonuke commented Jul 18, 2021

If we hard-coded the matrix, could we get more parallelism on the expensive builds?

@bam241
Copy link
Member Author

bam241 commented Jul 18, 2021

@gonuke I am happy with his.

package are tagged as stable when test pass (set on merge) https://github.com/bam241/DAGMC/pkgs/container/dagmc-ci-ubuntu-18.04-gcc-ext-hdf5_1.10.4-moab_develop

I also limited the retagging to svalinn only to limit the amount of pollution we are producing in users forks....

@bam241
Copy link
Member Author

bam241 commented Jul 18, 2021

If we hard-coded the matrix, could we get more parallelism on the expensive builds?

we already have parallelism for the 4 expensive builds

@bam241
Copy link
Member Author

bam241 commented Jul 18, 2021

see https://github.com/bam241/DAGMC/actions/runs/1042760146
the Matrix: build-deps-img box

@gonuke
Copy link
Member

gonuke commented Jul 18, 2021

If we hard-coded the matrix, could we get more parallelism on the expensive builds?

we already have parallelism for the 4 expensive builds

OK! I must have been looking at something old that looked slower.

@gonuke
Copy link
Member

gonuke commented Jul 18, 2021

I'll wait for CircleCI (should be OK) and then merge

@gonuke
Copy link
Member

gonuke commented Jul 18, 2021

Thanks for tackling this @bam241

@gonuke gonuke merged commit 2767342 into svalinn:develop Jul 18, 2021
@bam241 bam241 deleted the docker_test branch November 28, 2024 14:40
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.

3 participants