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

add option to force usage of fallback_auth_store #435

Merged
merged 9 commits into from
Dec 15, 2023

Conversation

0xbe7a
Copy link
Contributor

@0xbe7a 0xbe7a commented Dec 12, 2023

When running within a CI, it is sometimes easier not to rely on the native authentication toolchain, but to use a simple credentials file instead (see prefix-dev/rattler-build#334). This PR adds an option to force the use of this fallback file and disable the use of the system toolchain.

@wolfv
Copy link
Contributor

wolfv commented Dec 12, 2023

I think this is "OK" for me. Not a super-fan of the option :)

I am wondering if we should rather create another authentication storage that can be supplied instead of the current one. The new one would rely only on the fallback storage and uses plain JSON as database, and then we would have a Rust trait for an authentication storage?

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 12, 2023

Yeah, that sounds a bit more elegant.

@0xbe7a 0xbe7a requested a review from wolfv December 12, 2023 18:32
@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 13, 2023

I still need to test this with rattler-build

@wolfv
Copy link
Contributor

wolfv commented Dec 13, 2023

Nice work! Looks good to me. If we can get rid of anyhow it's perfect :)

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 13, 2023

I adapted and tested the rattler-build PR with our real-world example. Works great! I also changed the file location of the .json file, as the format changed slightly and we now store valid JSON.

@0xbe7a
Copy link
Contributor Author

0xbe7a commented Dec 13, 2023

Would you prefer to return a Box<dyn Error + Send + Sync>instead of an anyhow Result? Since I wanted to leave the option for users to bring your their own backend, you can't really type the error type.

@wolfv
Copy link
Contributor

wolfv commented Dec 13, 2023

right. let's try to ask @baszalmstra what he thinks is best.

Copy link
Collaborator

@baszalmstra baszalmstra 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, almost looks good to me. I left some small comments.

And I would just use anyhow Result.

@wolfv wolfv merged commit 8948ec7 into conda:main Dec 15, 2023
13 checks passed
@0xbe7a 0xbe7a deleted the force_fallback branch December 15, 2023 09:43
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