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

Log error when multiple remotes have the same org name #2228

Merged
merged 19 commits into from
Apr 10, 2023

Conversation

anoronh4
Copy link
Contributor

@anoronh4 anoronh4 commented Mar 29, 2023

Addressing #2157

PR checklist

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

@anoronh4 anoronh4 changed the title Feature/warn multiple remotes Warn when multiple remotes have the same org name Mar 29, 2023
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #2228 (3f44849) into dev (bb533fc) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #2228      +/-   ##
==========================================
+ Coverage   72.98%   73.05%   +0.06%     
==========================================
  Files          78       77       -1     
  Lines        8384     8342      -42     
==========================================
- Hits         6119     6094      -25     
+ Misses       2265     2248      -17     
Impacted Files Coverage Δ
nf_core/components/install.py 87.91% <100.00%> (+1.05%) ⬆️

... and 7 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@anoronh4 anoronh4 marked this pull request as ready for review March 30, 2023 01:53
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Thanks for this PR :)
I would throw an error instead of a warning in this case, and also reword a little bit the message to make it more clear.

nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
@anoronh4 anoronh4 requested a review from mirpedrol March 30, 2023 19:00
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM, only some last changes 🙂

nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
nf_core/components/install.py Outdated Show resolved Hide resolved
anoronh4 and others added 3 commits March 31, 2023 11:12
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
Co-authored-by: Júlia Mir Pedrol <mirp.julia@gmail.com>
@anoronh4
Copy link
Contributor Author

anoronh4 commented Mar 31, 2023

@mirpedrol i think the function check_alternate_remotes is doing what it's supposed to be doing, but i think i've CI checks might be complaining because the module tests literally use alternate remotes with the same org_path! I suspect that i will have to change https://gitlab.com/nf-core/modules-test.git so that the org_path: nf-core-test, but i'm not sure if that will work with all the tests. any reason why the content of that repo can't be held on the main nf-core/modules repo?

@mirpedrol
Copy link
Member

Some tests are failing due to log.err() which should be log.error().
This GitLab repo is used to test installing modules from other repos and from GitLab, so we still need to use it. For most of the failing tests, I think what we could do when installing modules in a pipeline is to remove all previously installed modules, the pipeline is created from the template which always includes fastqc and multiqc. And then add a test for this error. Sounds good?

@anoronh4
Copy link
Contributor Author

anoronh4 commented Apr 3, 2023

Some tests are failing due to log.err() which should be log.error(). This GitLab repo is used to test installing modules from other repos and from GitLab, so we still need to use it. For most of the failing tests, I think what we could do when installing modules in a pipeline is to remove all previously installed modules, the pipeline is created from the template which always includes fastqc and multiqc. And then add a test for this error. Sounds good?

sorry, i may have caused confusion, i found the log.err already and fixed locally only without committing and pushing, and when i tested locally the new error message started popping up. yes that sounds like it could work! i'll try to implement that. sorry for all the back and forth, hopefully we can get this moving!

@anoronh4 anoronh4 changed the title Warn when multiple remotes have the same org name Log error when multiple remotes have the same org name Apr 3, 2023
@anoronh4
Copy link
Contributor Author

anoronh4 commented Apr 7, 2023

@mirpedrol i have implemented the changes we last discussed and it seems to pass all checks. however, i wonder if the better solution would have been to change the org_path in the gitlab repo, that way less time is spent removing default modules, and we would still have the ability to test out how tools behaves with multiple remotes. Having spent so much time on the current solution, I wanted to get someone else's thoughts before pursuing something different. If this solution is preferred I'm happy to approve this PR as-is.

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.

None yet

2 participants