-
Notifications
You must be signed in to change notification settings - Fork 65
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
targeting 9c96d17 MOAB commit instead of version 5.1 #740
Conversation
@gonuke @pshriwise docker build stack is finally built. |
Can you test locally on the new docker stack? |
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 wonder if there is a way to structure these files to define a DAGMC_CI_MOAB_VERSION
in one place and then refer to it throughout??
I'll try to think of a way... the problem is that is not probably easy to have bash talking to Circle.... ( In the mean time I'll build and test DAGMC the 4 docker images built against MOAB |
successfully tested:
|
A first step might be to use the full branch name |
Would you like me to implement this here ? |
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.
A couple of optional thoughts on the way to streamlining this change
# MOAB Verions: 5.1.0 | ||
ARG MOAB=5.1.0 | ||
# MOAB Commit: 9c96d17 (Merged commit of @pshriwise thread fix) | ||
ARG MOAB=9c96d17 |
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.
doesn't this get overridden by the env variable when called via update_docker.sh
. Therefore, this could keep 5.1.0
as the default, but the value is changed in the script?
@@ -79,7 +79,7 @@ workflows: | |||
img: [ "16.04", "18.04"] | |||
compiler: ["clang", "gcc"] | |||
hdf5: ["1.10.4"] | |||
moab: ["5.1.0"] | |||
moab: ["9c96d17"] |
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 don't know enough about CI/yaml to know if this can be defined only once in this file and reused???
See the two small comments I just made and if you think they make sense, we can stop there and work to merge this. |
I think they make sense, but I'd like to do it cleanly all the way, instead of patching the file one after the other... I'll prepare a dedicated PR to do this! |
I think these are all the tests, right? So then we can merge this and push those images to dockerhub? |
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.
No comments from me. Thanks for addressing this @bam241!
Do you want to push these images first and then relaunch tests? |
sure will do! |
@pshriwise @gonuke docker stack is updated all tests are running fine |
Thanks @bam241 |
This is a temporary MOAB update in CI to allow our CI to pass.
this will be change at the next MOAB release
I am building the docker stack right now, I'll push them on request