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: supports reading config values from CLI #605

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

anthonykimani
Copy link

This PR works on issue #604.

I using the config-rs library to combine the configs in the main.rs file
image

I also set default values in the settings.rs file

image

image

@anthonykimani
Copy link
Author

you can the server with various options:

  1. Using defaults - cargo run
  2. Overriding Specific Settings - cargo run -- --port 8080 --tls-enabled false --log-level INFO
  3. Using Environment Variables - NOTARY_SERVER_SERVER__PORT=9000 cargo run

@yuroitaki yuroitaki self-requested a review September 26, 2024 03:51
Copy link
Member

@yuroitaki yuroitaki left a comment

Choose a reason for hiding this comment

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

Amazing stuff! That was really fast since I just put up the issue yesterday, thanks a lot! Just a few comments :)

crates/notary/server/src/settings.rs Show resolved Hide resolved
crates/notary/server/src/settings.rs Outdated Show resolved Hide resolved
crates/notary/server/src/settings.rs Outdated Show resolved Hide resolved
crates/notary/server/src/settings.rs Outdated Show resolved Hide resolved
.idea/.gitignore Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
crates/notary/server/src/settings.rs Outdated Show resolved Hide resolved
crates/notary/server/src/main.rs Outdated Show resolved Hide resolved
crates/notary/server/src/main.rs Outdated Show resolved Hide resolved
crates/notary/server/src/main.rs Outdated Show resolved Hide resolved
crates/notary/server/src/domain/cli.rs Outdated Show resolved Hide resolved
crates/notary/server/config/config.yaml Outdated Show resolved Hide resolved

tls:
enabled: true
private-key-pem-path: "./fixture/tls/notary.key"
certificate-pem-path: "./fixture/tls/notary.crt"
private_key_pem_path: "../fixture/tls/notary.key"
Copy link
Member

Choose a reason for hiding this comment

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

the path was correct, why did you change it?

Copy link
Author

@anthonykimani anthonykimani Sep 27, 2024

Choose a reason for hiding this comment

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

on my end the path didn't load, could you try with both relative paths on your end.

crates/notary/server/src/settings.rs Outdated Show resolved Hide resolved
@anthonykimani
Copy link
Author

Sorry @yuroitaki I meant to refactor the commit, I've included the changes you've suggested :) . This commit will load the config.yaml and can also override with cargo run -- --port 8080 --tls-enabled false --log-level INFO .

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.

2 participants