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: Use CA certificates from conda-forge::ca-certificates #3765

Merged
merged 16 commits into from
Jan 24, 2025

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan added the release::bug_fixes For PRs fixing bugs label Jan 22, 2025
@jjerphan jjerphan changed the title fix: Use CA certificates from the environment on Linux fix: Use CA certificates from conda-forge::ca-certificates Jan 22, 2025
@jjerphan jjerphan force-pushed the fix/ca-certificate-resolution branch from 1726bc9 to 1b17459 Compare January 22, 2025 17:59
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the fix/ca-certificate-resolution branch 3 times, most recently from 0df8b27 to bcb9d08 Compare January 23, 2025 11:55
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the fix/ca-certificate-resolution branch from bcb9d08 to 7205e24 Compare January 23, 2025 12:48
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the fix/ca-certificate-resolution branch from bd29417 to c35ccd0 Compare January 23, 2025 16:59
Signed-off-by: Julien Jerphanion <[email protected]>
@jjerphan jjerphan force-pushed the fix/ca-certificate-resolution branch from c35ccd0 to 66d72d9 Compare January 23, 2025 18:44
@jjerphan jjerphan marked this pull request as ready for review January 23, 2025 20:38
Copy link
Member

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

LGTM despite my doubts about the Context root_prefix member. Maybe @Klaim has
a better understanding of the initialization of these variables.

@@ -177,7 +178,7 @@ namespace mamba
Context::Context(const ContextOptions& options)
{
on_ci = static_cast<bool>(util::get_env("CI"));
prefix_params.root_prefix = util::get_env("MAMBA_ROOT_PREFIX").value_or("");
prefix_params.root_prefix = detail::get_root_prefix();
Copy link
Member

Choose a reason for hiding this comment

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

That's the only part I'm not sure about, since prefix_params.root_prefix is used to store the root_prefix set by the config (see https://github.com/mamba-org/mamba/blob/main/libmamba/src/api/configuration.cpp#L1237-L1245). I don't know whether some parts of the code do something specific when this variable is empty, nor if there are use cases where it would be valid to have it empty (when using the library out of mamba / micromamba for instance).

@jjerphan jjerphan merged commit 7fa748f into mamba-org:main Jan 24, 2025
34 checks passed
@jjerphan jjerphan deleted the fix/ca-certificate-resolution branch January 24, 2025 09:44
@Klaim
Copy link
Member

Klaim commented Jan 24, 2025

LGTM despite my doubts about the Context root_prefix member. Maybe @Klaim has a better understanding of the initialization of these variables.

I dont think I have more understanding here, although I share your suspicion. A cursory look at how that variable is used suggests to me that most of libmamba's code relies on this variable to not be empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release::bug_fixes For PRs fixing bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants