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

feat: use RATTLER_AUTH_FILE and use proper reqwest clients in search #994

Merged
merged 9 commits into from
Mar 19, 2024

Conversation

wolfv
Copy link
Member

@wolfv wolfv commented Mar 16, 2024

@pavelzw do you think you could try this branch?

I tested a bit and I could not see what would go wrong with your config file though.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 16, 2024

Just tested it out, it works with this version 🥳 Thanks @wolfv!
Closes #990
Closes #989

@wolfv
Copy link
Member Author

wolfv commented Mar 16, 2024

I am wondering if we should make this something supported in rattler itself. In that case, we could also make it so that

export RATTLER_AUTH_FILE=test.json
pixi auth login prefix.dev --token bla

would write to the RATTLER_AUTH_FILE.

But we can also do that in a follow up PR.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 16, 2024

It works for pixi search, pixi add and pixi install.

For pixi global install i get the same error message:

image

@wolfv
Copy link
Member Author

wolfv commented Mar 17, 2024

I think my last commit fixes authentication for global install. We should also take a look to make sure that the default channels are used (by default) for the global install command.

@pavelzw
Copy link
Contributor

pavelzw commented Mar 17, 2024

We should also take a look to make sure that the default channels are used

ideally also for pixi search (if you're not in a pixi project where the channels should be used from pixi.toml)

Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

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

Two questions

@@ -12,6 +15,21 @@ pub fn default_retry_policy() -> ExponentialBackoff {
ExponentialBackoff::builder().build_with_max_retries(3)
}

fn auth_middleware() -> AuthenticationMiddleware {
if let Ok(auth_file) = std::env::var("RATTLER_AUTH_FILE") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this file location also be part of the global config?

Copy link
Member Author

Choose a reason for hiding this comment

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

I made it so! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you document it aswell

src/utils/reqwest.rs Show resolved Hide resolved
@ruben-arts
Copy link
Contributor

Except for my documentation and better error request it does work! If that is added we can merge.

@ruben-arts ruben-arts enabled auto-merge (squash) March 19, 2024 08:59
@ruben-arts ruben-arts merged commit 2d0e7df into prefix-dev:main Mar 19, 2024
17 checks passed
@william-alhant
Copy link

I think this change is not fully functionnal. I am setting the environment variable but it does not work from what I can see.
The new option does not seem to be taken into account by shell-hook.
Additionally, it seems build_reqwest_clients is called with the config before with_cli_config, so the auth_file option is ineffective.

@wolfv
Copy link
Member Author

wolfv commented Mar 19, 2024

@william-alhant can you check on main again? We just merged another PR that re-shuffled a few things again and should make things work smoother.

@wolfv wolfv deleted the use-rattler-auth branch March 19, 2024 16:02
@william-alhant
Copy link

I just checked again on main (93f838b). The issue is still there.

@wolfv
Copy link
Member Author

wolfv commented Mar 19, 2024

@william-alhant if you have the time, maybe you could give #1019 a try

@william-alhant
Copy link

@wolfv Thank you, I gave #1019 a try, and with this change RATTLER_AUTH_FILE is indeed effective :)

@wolfv
Copy link
Member Author

wolfv commented Mar 19, 2024

great!

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.

4 participants