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

PR: Allow activate-environment to be a path #137

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

bollwyvl
Copy link
Contributor

This follows up on #136 where we might want to allow for activate-environment to be a --prefix rather than a --name argument to various conda commands.

Also removes some dead code which was using --name, and tweaks some more docs and typos in log messages.

Of worrying note: I have now seen at least one 403 from the GitHub API when requesting the miniforge releases, despite my best efforts at caching. We probably need to:

@goanpeca
Copy link
Member

goanpeca commented Dec 30, 2020

@bollwyvl I did not understand the issues before, querying Gtihub API is expensive as you have now noticed.

Why do we need the miniforge releases?

We should just build the URL from what the user requests (variants etc) and fail with unknown version if it fails, no need to provide all possible releases withe the API query (if that was the purpose of it?)

@bollwyvl
Copy link
Contributor Author

Why do we need the miniforge releases?

At present, miniforge-version is not required, though of course we could: demanding a version seems like it might be less nice for the simplest use case. Unless we do, we need to get a version from somewhere as there is no latest at a predictable URL. Having the page on the above linked issue would give us that, as would hard-coding a version-of-last-resort... it seems to be getting released more often than miniconda.

@goanpeca
Copy link
Member

goanpeca commented Dec 30, 2020

At present, miniforge-version is not required, though of course we could: demanding a version seems like it might be less nice for the simplest use case.

Might be less nice, but is what you have to provide to overcome the unnecessary problems with the API and havig a page with all that infi (not happenning soon form what I just read?) so I prefer going with explicit over implicit here :)

we have miniconda-version already, so having a miniforge-version seems only natural, and would save us from any issues with APIs. If another source of more reliable info becomes available in the future we can the improve using that but for 2.1 I would rather have the miniforge-version requirement.

@jaimergp
Copy link
Member

This might not be as nice and convenient for the user but what if we deprecate miniconda-version, miniforge-version, miniforge-variant and friends and just expose installer? This could be either a URL to a constructor-derived installer, or a keyword like Miniconda-4.12 or Mambaforge-4.12.

If finding the right installer URL is tricky, we could collect a list in our up and coming fancy docs.

@bollwyvl
Copy link
Contributor Author

rather have the miniforge-version requirement.

Sure, sounds good. seems out of scope for this PR, will do another... one thing we can do then is have a default miniforge-variant of Miniforge, so it would still only be a single input to get "vanilla" conda/cpython at a particular version.

@goanpeca
Copy link
Member

Sure, sounds good. seems out of scope for this PR, will do another... one thing we can do then is have a default miniforge-variant of Miniforge, so it would still only be a single input to get "vanilla" conda/cpython at a particular version.

Agreed!


@jaimergp let's explore our options on another issue :-p

@goanpeca
Copy link
Member

Is this ready for review @bollwyvl ?

@bollwyvl
Copy link
Contributor Author

deprecate miniconda-version, miniforge-version, miniforge-variant

i feel like deprecation makes people sad... because fixing ci that used to work makes people sad. and the URL structures are fairly different between them, anyhow, even though they both come out of constructor. another reason i like the separate ones is that it becomes easy to use github search for examples when you have semantically-rich strings.

@bollwyvl
Copy link
Contributor Author

Is this ready for review

yeah, unless we want to sneak anything else into the tests... i guess I haven't tried giving it a base env, might break the world...

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Dec 30, 2020

oh, and @goanpeca, re: #136: can you comment on why we always add $CONDA/condabin to $PATH?

@bollwyvl
Copy link
Contributor Author

bollwyvl commented Dec 31, 2020

Looks like we need to keep condabin... will revert... specifically, was needed for bash on windows... not exactly #136, but i don't sowing future problems windows support issues...

@bollwyvl bollwyvl force-pushed the add-activate-by-prefix branch from 19f36f1 to c849891 Compare December 31, 2020 00:00
inputs.activateEnvironment !== "base" &&
inputs.activateEnvironment !== "root" &&
inputs.activateEnvironment !== "";
const isValidActivate = !utils.isBaseEnv(inputs.activateEnvironment);
Copy link
Member

Choose a reason for hiding this comment

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

is utils.isBaseEnv also checking for an empt string env?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


/** Names for a conda `base` env */
export const BASE_ENV_NAMES = ["root", "base", ""];

Copy link
Member

@goanpeca goanpeca left a comment

Choose a reason for hiding this comment

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

One question, otherwise looks good :)

Thanks @bollwyvl !

@goanpeca goanpeca changed the title [wip] allow activate-environment to be a path PR: Allow activate-environment to be a path Dec 31, 2020
@bollwyvl bollwyvl mentioned this pull request Dec 31, 2020
5 tasks
@goanpeca goanpeca merged commit ef9cc05 into conda-incubator:master Dec 31, 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