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

fix 343: add conda_default_allow_channels option #356

Closed
wants to merge 5 commits into from

Conversation

csadorf
Copy link

@csadorf csadorf commented Jul 17, 2022

If set, automatically add new channels to the channels database.

Before merge:

  • Implement tests
  • Add documentation

Closes #343.

If set, automatically add new channels to the channels database.
@csadorf
Copy link
Author

csadorf commented Jul 17, 2022

The new option is set to "True" by default which breaks previous behavior.

@csadorf
Copy link
Author

csadorf commented Jul 17, 2022

@costrouc I think this is more or less ready, however I think we should at least consider whether maybe instead of adding an additional option, setting the conda_allowed_channels to an empty list to achieve the same behavior would be more intuitive to users.

I also could use some guidance on the test implementation.

@csadorf csadorf marked this pull request as draft July 17, 2022 15:33
@csadorf
Copy link
Author

csadorf commented Jul 17, 2022

Manually tested with:

channels:
- conda-forge
- bioconda
- defaults
dependencies:
- _libgcc_mutex=0.1
- _openmp_mutex=4.5
- bcftools=1.10.2
- blis=0.7.0
- pip:
  - nothing
- ipykernel
name: map-and-call
prefix: null

Copy link
Member

@costrouc costrouc left a comment

Choose a reason for hiding this comment

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

I saw some Black formatting issues and just added two comments on a conditional check that needs to be made. Otherwise this PR looks great to me. Thanks for your work @csadorf

conda-store-server/conda_store_server/build.py Outdated Show resolved Hide resolved
conda-store-server/conda_store_server/build.py Outdated Show resolved Hide resolved
@csadorf csadorf marked this pull request as ready for review July 22, 2022 08:43
@csadorf csadorf requested a review from costrouc July 22, 2022 08:43
@csadorf
Copy link
Author

csadorf commented Jul 22, 2022

@costrouc I'd suggest that you consider my suggestion concerning the overloading of the conda_allowed_channels trait, but otherwise this should be ready to merge.

@costrouc
Copy link
Member

Thanks @csadorf. Yeah I get what you are saying and that makes sense. I think it would be a good idea to write it like that especially since there is no case where you would want an empty list of channels in the first place. Making it so that we can overload that behavior!

Would you mind making that change? Sorry for the late response

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.

Allow new channels to be added dynamically
2 participants