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.

Sign up for GitHub

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

Jump to bottom

config: add secret path env var #135

Merged
merged 1 commit into from
Jun 12, 2023
Merged

config: add secret path env var #135

merged 1 commit into from
Jun 12, 2023

Conversation

happysalada
Copy link
Contributor

hey, I had a go at #132
this is just an early attempt.
I've added a flake.nix which gives a development environment for anyone wanting to contribute. Happy to remove it in case you don't like it.
I've added what I thought would be a solution for reading secrets from path.
I haven't optimize the allocation, this should probably be a Cow, but I wanted to see how you felt about this.
Note that there is a bunch of clippy warnings that I didn't touch.
Let me know if anything, I'm flexible.

s3rius
s3rius reviewed Jun 12, 2023
View reviewed changes
src/storages/models/available_stores.rs Outdated
let mut contents = String::new();
buf_reader
.read_to_string(&mut contents)
.unwrap_or_else(|_| panic!("failed to open path {}", path.display()));
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
.unwrap_or_else(|_| panic!("failed to open path {}", path.display()));
.unwrap_or_else(|_| panic!("failed to read from path {}", path.display()));

s3rius
s3rius reviewed Jun 12, 2023
View reviewed changes
flake.nix Outdated
@@ -0,0 +1,103 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Aren't flakes are NixOs only feature? How to use it?

s3rius
s3rius reviewed Jun 12, 2023
View reviewed changes
src/config.rs Outdated Show resolved Hide resolved
s3rius
s3rius requested changes Jun 12, 2023
View reviewed changes
Copy link
Owner

@s3rius s3rius left a comment

Choose a reason for hiding this comment

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

Looks good. But I'm not sure about flake, because as any other rust project, you can simply install it with cargo install with no additional commands.

Also, can you please update documentation for new variables?

@happysalada happysalada force-pushed the add_secret_path_env_var branch from 2252f1e to 490ca43 Compare June 12, 2023 18:06
@happysalada
Copy link
Contributor Author

@s3rius I've had a go at it, let me know if I forgot something.

@happysalada
Copy link
Contributor Author

I've removed the flake.nix. However this is not just a nixos feature. I'm on macos, you just need the nix package manager to benefit from it.
This sets up a development environment with a precise rust version, openssl version... Those mostly work out of the box, but sometimes they don't, it's just a way to make sure everyone has the same environment no matter what is installed on their machine.
Don't worry about it though, I've removed it, it's simpler that way.

s3rius
s3rius requested changes Jun 12, 2023
View reviewed changes
Copy link
Owner

@s3rius s3rius left a comment

Choose a reason for hiding this comment

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

Everything with docs look nice! Thank you. I just took a closer look to from_string_or_path function. It looks good and I'm pretty sure it works, but I guess it may be even better.

src/storages/models/available_stores.rs Show resolved Hide resolved
src/storages/models/available_stores.rs Outdated Show resolved Hide resolved
src/storages/models/available_stores.rs Outdated Show resolved Hide resolved
@happysalada happysalada force-pushed the add_secret_path_env_var branch from 490ca43 to f5eb52b Compare June 12, 2023 20:46
@happysalada
Copy link
Contributor Author

@s3rius I agree with all of your comments except maybe the match.
The match with 2 branches I feel is less readable than the if else.
Let me know if anything of course.

@s3rius
Copy link
Owner

s3rius commented Jun 12, 2023

Okay. Agree, that match can be not as readable as if else here.

@happysalada happysalada force-pushed the add_secret_path_env_var branch from f5eb52b to 90a8f39 Compare June 12, 2023 22:03
@happysalada
Copy link
Contributor Author

There was a clippy lint failing, I've just force pushed to fix it.

@s3rius s3rius changed the base branch from master to develop June 12, 2023 22:06
@s3rius s3rius merged commit 4a74ba8 into s3rius:develop Jun 12, 2023
@happysalada happysalada deleted the add_secret_path_env_var branch June 12, 2023 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s3rius s3rius s3rius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@happysalada @s3rius