-
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
Switching from CI to GitHub #752
Conversation
@gonuke @pshriwise @shimwell I think this is ready for a first round of review. this should port our CircleCI capability to GithubAction |
.github/workflows/build_test.yml
Outdated
- name: Building DAGMC | ||
run: | | ||
cd /root/build_dir/DAGMC | ||
CI/circleci/install.sh |
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.
Perhaps we should remove the circleci
folder or rename 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.
renamed it scripts
as it live in the CI folder already
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.
Thanks @bam241!
A few comments, mostly as I try to learn the syntax and develop a mental model
.github/workflows/build_test.yml
Outdated
container: | ||
image: svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest | ||
steps: | ||
- name: Install latest git |
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.
?? we have to install git ??
Or is it just the newer version we need - maybe @shimwell 's ghcr.io images have it already?
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.
The dockerhub and ghcr docker images appears to have the same git version.
sudo docker run -it svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest
git --version
>> version 2.17.1
sudo docker run -it ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest
git --version
>> version 2.17.1
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.
They do.
But we need 2.18 minimum so GitHub action use it for the checkout.
Otherwise it falls back on the "REST" api and just download a folder that does not contain git versioning info
We need those for submodule and housekeeping
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.
Perhaps the actions/checkout@v2
can also do submodules checkout but they are just off by default. I think it can be switched on (in the usage section of docs -> https://github.com/actions/checkout)
.github/workflows/build_test.yml
Outdated
|
||
- name: Building DAGMC | ||
run: | | ||
cd /root/build_dir/DAGMC |
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.
consistency: above, you used $GITHUB_WORKSPACE
and here you used /root/build_dir/DAGMC
.github/workflows/build_test.yml
Outdated
run: | | ||
apt-get install -y software-properties-common | ||
add-apt-repository ppa:git-core/ppa | ||
apt update | ||
apt install -y git | ||
|
||
- name: Checkout repository | ||
uses: actions/checkout@v2 |
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.
This might be worth a try as if it works the parts were updated git installs are used could be removed
run: | | |
apt-get install -y software-properties-common | |
add-apt-repository ppa:git-core/ppa | |
apt update | |
apt install -y git | |
- name: Checkout repository | |
uses: actions/checkout@v2 | |
- name: Checkout repository | |
uses: actions/checkout@v2 | |
with: | |
submodules: 'true' |
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.
already tried this does not work as git version is < 2.18
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.
see here: https://github.com/svalinn/DAGMC/runs/3031336027?check_suite_focus=true
Run actions/checkout@v2
2 with:
3 submodules: recursive
4 repository: svalinn/DAGMC
5 token: ***
6 ssh-strict: true
7 persist-credentials: true
8 clean: true
9 fetch-depth: 1
10 lfs: false
11 env:
12 hdf5_versions: 1.10.4
13 hdf5_build_dir: hdf5_build_dir
14 /usr/bin/docker exec 47d233d87e73e379239b48b5ef537b79ae073afeb55c7f2006b218366d7d4468 sh -c "cat /etc/*release | grep ^ID"
15 Syncing repository: svalinn/DAGMC
16 Getting Git version info
20 Deleting the contents of '/__w/DAGMC/DAGMC'
21 The repository will be downloaded using the GitHub REST API
22 To create a local Git repository instead, add Git 2.18 or higher to the PATH
23 Error: Input 'submodules' not supported when falling back to download using the GitHub REST API. To create a local Git repository instead, add Git 2.18 or higher to the PATH.
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.
Can we add the git update to the Dockerfiles, then?
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.
yes I believe that's the way to go.
I added it there to make sure we all agreed on this solution :)
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.
See #753
Co-authored-by: Jonathan Shimwell <drshimwell@gmail.com>
Do we know why these are failing? |
Looks like we are trying to pull an image like this I think it might be named slightly differently |
Housekeeping: | ||
runs-on: ubuntu-latest | ||
container: | ||
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:stable |
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.
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:stable | |
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest |
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.
We want stable. They are building right now.
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.
Just that latest is also the default. So if you do a docker pull without anything at the end it will grab the latest one as latest is also default.
The way I shaped this is the following: docker building ci will use the latest tag as temporary tag to try compiling and the tests for dagmc. (This will run on the account of the user issuing the PR) On merge, this will build with latest tag, run the test against the newly latest image. |
Thanks for the work on this transition @bam241 |
This failed on merge - we need the same fix as #757 |
The logic will get complicated if we take the same approach a s #757 because in some events we want Modularity is the answer - make our own |
moving away from CircleCI