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

2.6.1 #222

Merged
merged 26 commits into from
Jun 23, 2020
Merged

2.6.1 #222

merged 26 commits into from
Jun 23, 2020

Conversation

maxulysse
Copy link
Member

nf-core/sarek pull request

Many thanks for contributing to nf-core/sarek!

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!
  • If necessary, also make a PR on the nf-core/sarek branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: CONTRIBUTING.md

@maxulysse maxulysse added the release This PR is for a release label Jun 22, 2020
@maxulysse maxulysse requested review from ggabernet and a team June 22, 2020 15:02
@apeltzer
Copy link
Member

Yeah the PR comes from your branch so the branch checks fail for a reason, shouldn't be bad ;-)

Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

I just have two comments in main.nf that I could not add directly as these lines have not been changed.

The rest is not important, we can also adapt it once the AWS has actually full tests

.github/workflows/awsfulltest.yml Outdated Show resolved Hide resolved
.github/workflows/awsfulltest.yml Outdated Show resolved Hide resolved
@ggabernet
Copy link
Member

In line 2709 of main.nf :
The indentation is weird.

And in line 2710 of main.nf a channel is created that I think it is not used afterward (or I could not find it).

CHANGELOG.md Show resolved Hide resolved
@maxulysse
Copy link
Member Author

In line 2709 of main.nf :
The indentation is weird.

And in line 2710 of main.nf a channel is created that I think it is not used afterward (or I could not find it).

Fixed your comments with #223 that was just merged.
And I updated this branch.

So I think we're good to go ;-)

@ggabernet
Copy link
Member

ok, great! Is it normal that one of the CI tests (annotation) fails?

@maxulysse
Copy link
Member Author

ok, great! Is it normal that one of the CI tests (annotation) fails?

I don't know why the circleci was not triggered on this PR.
Maybe it's because it's coming from my fork.

But basically the images for the annotation are not built, but they will be built once the PR is merged.

Worst case scenario, I can still build them all myself.

I can actually build the one for the tests now if you like.

Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

LGTM!

@ggabernet
Copy link
Member

All good, whatever works best for you :)

@maxulysse maxulysse merged commit bce378e into nf-core:master Jun 23, 2020
@maxulysse maxulysse deleted the 2.6.1 branch July 23, 2020 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release This PR is for a release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants