Skip to content

[ci] adding github action (closes #156) #159

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

Merged
merged 8 commits into from
Aug 7, 2020

Conversation

djarecka
Copy link
Collaborator

@djarecka djarecka commented Aug 7, 2020

@mgxd, @effigies @satra - testing github actions, happy to get any feedback (especially if you have better idea how to deal with container management/caching)

and I'd really want to know how to enable it... when I was testing on my profile I haven't had to do anything extra, it just worked when i included .github/workflows, but here it doesn't (at least for this pr)

this is a ga dashboard from my fork (tests are failing right now):
https://github.com/djarecka/nipype_tutorial/runs/957128296?check_suite_focus=true

EDIT
general comments / for future consideration

  • I've decided to use multiple jobs in order to be able to run things in parallel
  • I didn't find THE way of passing the image to the next job, so I've decided to use uses: satackey/action-docker-layer-caching@v0.0.5 for now (didn't check if pull/push from dockerhub is not faster). I'm assuming at some point this will be better supported and we will want to change it

@djarecka djarecka changed the title [ci] adding github action [ci] adding github action (closes #156) Aug 7, 2020
@satra
Copy link
Contributor

satra commented Aug 7, 2020

this is still testing via circle. actions only start executing after they have at least been added to master. so if it works for you, merge this in and then the action will fire. after that PRs should do the right thing.

Copy link
Contributor

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Cool! It seems docker caching as a whole is still in its infancy, so I'm not too sure there..

Dockerfile Outdated
@@ -9,6 +10,8 @@

FROM neurodebian:stretch-non-free

USER root
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't the user originally root by default?

branches: [ master ]
pull_request:
branches: [ master ]

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
schedule:
- cron: "0 10 * * *"

djarecka and others added 5 commits August 7, 2020 11:26
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
Co-authored-by: Mathias Goncalves <goncalves.mathias@gmail.com>
@djarecka
Copy link
Collaborator Author

djarecka commented Aug 7, 2020

a fun fact - the current version of the test_notebooks (2 years old?) where giving green light on CI regardless if tests were passing or failing...
see for example run_tests section of circleCI: https://app.circleci.com/pipelines/github/miykael/nipype_tutorial/87/workflows/cbc2e7db-0dd5-4352-bd9c-fb767a6b7975/jobs/1352

@mgxd
Copy link
Contributor

mgxd commented Aug 7, 2020

@djarecka it looks like the problem lies within the test script:

pytest_cmd = 'pytest --nbval-lax --nbval-cell-timeout 7200 -v -s %s' % test
print(pytest_cmd)
os.system(pytest_cmd)

since the return value of the system call is not checked, any command would pass.

@djarecka
Copy link
Collaborator Author

djarecka commented Aug 7, 2020

@mgxd - yes, I noticed... Will fix this later, and than would have to review the notebooks that are failing, don't even want to know when they started failing....

@djarecka
Copy link
Collaborator Author

djarecka commented Aug 7, 2020

actually, I might just merge this first to have at least working CI structure (i will give up on finding out why CircleCI doesn't want to build the image for me)

@djarecka djarecka merged commit 6f678cb into miykael:master Aug 7, 2020
@djarecka
Copy link
Collaborator Author

djarecka commented Aug 7, 2020

@satra - you were right, it works now! :)
in my fork i didn't need to merge to master, but perhaps it makes sense to treat owners of the repo differently

@djarecka djarecka mentioned this pull request Aug 7, 2020
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