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

Switching from CI to GitHub #752

Merged
merged 106 commits into from
Jul 20, 2021
Merged

Switching from CI to GitHub #752

merged 106 commits into from
Jul 20, 2021

Conversation

bam241
Copy link
Member

@bam241 bam241 commented Jul 9, 2021

moving away from CircleCI

@bam241 bam241 changed the title removing circle to save resource in CI Switching from CI to GitHub Jul 9, 2021
@bam241
Copy link
Member Author

bam241 commented Jul 9, 2021

@gonuke @pshriwise @shimwell I think this is ready for a first round of review.

this should port our CircleCI capability to GithubAction

- name: Building DAGMC
run: |
cd /root/build_dir/DAGMC
CI/circleci/install.sh
Copy link
Member

@shimwell shimwell Jul 9, 2021

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

Copy link
Member Author

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

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.

Thanks @bam241!

A few comments, mostly as I try to learn the syntax and develop a mental model

container:
image: svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest
steps:
- name: Install latest git
Copy link
Member

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?

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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 Show resolved Hide resolved
.github/workflows/build_test.yml Show resolved Hide resolved

- name: Building DAGMC
run: |
cd /root/build_dir/DAGMC
Copy link
Member

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 Show resolved Hide resolved
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
Copy link
Member

@shimwell shimwell Jul 10, 2021

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

Suggested change
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'

Copy link
Member Author

@bam241 bam241 Jul 12, 2021

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

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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 :)

Copy link
Member

Choose a reason for hiding this comment

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

See #753

@gonuke
Copy link
Member

gonuke commented Jul 19, 2021

Do we know why these are failing?

@shimwell
Copy link
Member

Do we know why these are failing?

Looks like we are trying to pull an image like this
docker pull ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:stable

I think it might be named slightly differently
docker pull ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest

Housekeeping:
runs-on: ubuntu-latest
container:
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:stable
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
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:stable
image: ghcr.io/svalinn/dagmc-ci-ubuntu-18.04-housekeeping:latest

Copy link
Member Author

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.

Copy link
Member

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.

@bam241
Copy link
Member Author

bam241 commented Jul 19, 2021

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.
If test are successful the image will be tagged as stable. And we will use those for ci.

@bam241
Copy link
Member Author

bam241 commented Jul 19, 2021

@gonuke @shimwell I just realized that latest might be a confusion tag for temporary image shall I use something else ?

@bam241
Copy link
Member Author

bam241 commented Jul 20, 2021

@gonuke @shimwell I just realized that latest might be a confusion tag for temporary image shall I use something else ?

I'll do that in a dedicated PR. (opened an issue #762 )
If you are fine with the current state of this, we can merge this and retire our CircleCI hooks....

@gonuke
Copy link
Member

gonuke commented Jul 20, 2021

Thanks for the work on this transition @bam241

@gonuke gonuke merged commit fbf2af9 into svalinn:develop Jul 20, 2021
@gonuke
Copy link
Member

gonuke commented Jul 20, 2021

This failed on merge - we need the same fix as #757

@gonuke
Copy link
Member

gonuke commented Jul 20, 2021

The logic will get complicated if we take the same approach a s #757 because in some events we want BuildTest to depend on Housekeeping and in other events we do not.

Modularity is the answer - make our own BuildTest action and call it in different workflows. (do we want to go down this road right now???)

@shimwell
Copy link
Member

@gonuke @shimwell I just realized that latest might be a confusion tag for temporary image shall I use something else ?

Yes please.

@bam241 bam241 deleted the ci2GA 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