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

Add GitHub Actions to Template #369

Merged
merged 18 commits into from Nov 29, 2019
Merged

Add GitHub Actions to Template #369

merged 18 commits into from Nov 29, 2019

Conversation

maxulysse
Copy link
Member

@maxulysse maxulysse commented Aug 23, 2019

Many thanks to contributing to nf-core/tools!

Please fill in the appropriate checklist below (delete whatever is not relevant). These are the most common things requested on pull requests (PRs).

PR checklist

  • This comment contains a description of changes (with reason)
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: https://github.com/nf-core/tools/tree/master/.github/CONTRIBUTING.md

@maxulysse maxulysse added automation linting pipeline-testing template nf-core pipeline/component template labels Aug 28, 2019
@maxulysse
Copy link
Member Author

This PR is based on nf-core/sarek#27

@maxulysse maxulysse marked this pull request as ready for review August 28, 2019 14:11
@maxulysse maxulysse mentioned this pull request Aug 28, 2019
8 tasks
@ewels
Copy link
Member

ewels commented Aug 28, 2019

SVG images are missing... But could probably just use shields.io anyway?

Travis stuff that I think we're still missing:

  • Correct branch logic
    • If not from this repo, then must not be against master
    • If from repo and against master, then coming from dev
  • That the changelog has been updated (maybe this is only the main tools repo, not pipelines?)

Would be good to have the branch stuff in it's own file / test I think? Would be super quick then and I like having the tests split up like that to make it super clear..

Great work!

@ewels
Copy link
Member

ewels commented Aug 28, 2019

From here:

You should put the following into your README which is supported directly by GitHub:

[![Actions Status](https://github.com/{owner}/{repo}/workflows/{workflow_name}/badge.svg)](https://github.com/{owner}/{repo}/actions)

@maxulysse
Copy link
Member Author

maxulysse commented Aug 29, 2019

Would be good to have the branch stuff in it's own file / test I think? Would be super quick then and I like having the tests split up like that to make it super clear..

Good point, I totally forgot about that one, I'll add that to the test on my sarek PR

From here:

That's exactly the link I used, I just figured one need to replace {owner}, {repo} and {workflow_name}, but I'm guessing it might have been working with the old Actions, and not this one, I'll keep looking

EDIT: Never mind, I found out how it works... It's pretty tricky in fact, for workflow_name you have to put indeed the workflow_name, wich is the name that you give the workflow in the yml file, and not the name of the yml file itself...

Here is how it works for Nextflow (the yml file being build.yml):
![Actions Status](https://github.com/nextflow-io/nextflow/workflows/Nextflow%20CI/badge.svg)
Actions Status

@maxulysse
Copy link
Member Author

Nothing on shields.io yet, if I understood this issue: badges/shields#2574

@maxulysse
Copy link
Member Author

I fixed the badges, and added a test to protect master: https://github.com/nf-core/tools/blob/2a959e24b530c551d52ce42c4c2b22a73e65a55e/nf_core/pipeline-template/%7B%7Bcookiecutter.name_noslash%7D%7D/.github/workflows/branch.yml

It's only triggered for master branch, and check that the GITHUB_AUTHOR is nf-core and that the head branch is dev, which should ensure the same test we were doing.

It should need further testing to verify that, so do not merge until we're sure with the Sarek PR.

Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Only question remaining for me: Are we dropping TravisCI in favor for this? Looks faster, more streamlined, better implemented and once you grasp the concept I don't see a reason to keep TravisCI tests in :D

@maxulysse
Copy link
Member Author

I would say yes in the long term, but not now, it's still in beta :-D

@apeltzer apeltzer modified the milestones: 1.7.1, 1.8 Oct 14, 2019
…b/workflows/ci.yml

Co-Authored-By: Phil Ewels <phil.ewels@scilifelab.se>
@apeltzer apeltzer requested a review from ewels November 27, 2019 11:04
@apeltzer apeltzer changed the title Add GitHub Actions Add GitHub Actions to Template Nov 27, 2019
@maxulysse
Copy link
Member Author

Quick question about the branch protection.
So basically we allow PR to master only from dev, except if it's a patch/fix.
Should we have a specific name for such a branch?
Should have some docs about how to patch a pipeline?

@ewels
Copy link
Member

ewels commented Nov 27, 2019

Should we have a specific name for such a branch?

We do, it's patch right?

- '[ $TRAVIS_PULL_REQUEST = "false" ] || [ $TRAVIS_BRANCH != "master" ] || ([ $TRAVIS_PULL_REQUEST_SLUG = $TRAVIS_REPO_SLUG ] && ([ $TRAVIS_PULL_REQUEST_BRANCH = "dev" ] || [ $TRAVIS_PULL_REQUEST_BRANCH = "patch" ]))'

[ $TRAVIS_PULL_REQUEST_BRANCH = "patch" ]

@maxulysse
Copy link
Member Author

maxulysse commented Nov 27, 2019

Should we have a specific name for such a branch?

We do, it's patch right?

I know, the important part of the question was:

Should have some docs about how to patch a pipeline?

@apeltzer
Copy link
Member

Yeah, having some docs on what / how we patch a pipeline would be good 👍

It shouldn't be used too often, though I hope 😆

@maxulysse
Copy link
Member Author

I updated the CONTRIBUTING.md and related files.

@maxulysse
Copy link
Member Author

Anyone has any idea why tests are failing on that one?

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Couple of double cookiecutters are breaking the templating..

@maxulysse
Copy link
Member Author

🤦‍♂️
Thanks a lot, I'm sure I was about to get there...

Co-Authored-By: Phil Ewels <phil.ewels@scilifelab.se>
@maxulysse
Copy link
Member Author

@ewels what do you think about the mergability of this one?

@ewels
Copy link
Member

ewels commented Nov 29, 2019

I'll review this if you guys review #467 and #465 😉

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Looks great! I added a few minor suggested changes for docs and comment tweaks, but that's all.

Note that we'll need to update nf-core bump-version and nf-core lint, and also remove .travis.yml as part of that, but we can do that in a separate PR I guess.

Thanks to @ewels for useful suggestions

Co-Authored-By: Phil Ewels <phil.ewels@scilifelab.se>
@ewels ewels merged commit d4d0039 into nf-core:dev Nov 29, 2019
@maxulysse maxulysse deleted the GithubActions branch November 29, 2019 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation linting pipeline-testing template nf-core pipeline/component template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants