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

Start with env variables from Configuration with Debug/Non Debug mode. #62

Merged
merged 20 commits into from Feb 10, 2022

Conversation

thienpow
Copy link
Contributor

@thienpow thienpow commented Feb 7, 2022

No description provided.

Copy link
Owner

@mateusfreira mateusfreira 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 the MR lots of great improvements here

I left a few recommendations of change let me know if some of them do not seem clear.

src/lib/disk_ops.rs Outdated Show resolved Hide resolved
src/lib/disk_ops.rs Outdated Show resolved Hide resolved
src/lib/disk_ops.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
src/lib/commad_line/commands.rs Outdated Show resolved Hide resolved
src/lib/commad_line/commands.rs Outdated Show resolved Hide resolved
src/lib/configuration.rs Outdated Show resolved Hide resolved
src/lib/configuration.rs Outdated Show resolved Hide resolved
src/lib/configuration.rs Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Owner

@mateusfreira mateusfreira left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution, @thienpow we are getting there.

I have tested the code locally and left a couple of other observations.

The more important ones are :

  1. that the user and password should consider command-line parameters and env var before stopping the execution.

  2. In the disk ops, we are building the config object all the time to pick a single property that we need.

I have made some changes to this PR; please pull from thienpow-master before pushing the new changes.

nun_tcp_addr: expect_env_var("NUN_HTTP_ADDR", "0.0.0.0:3014"),
nun_replicate_addr: expect_env_var("NUN_REPLICATE_ADDR", ""),
nun_log_level: expect_env_var("NUN_LOG_LEVEL", "Info"), //(Off, Error, Warn, Info, Debug, Trace)
nun_log_dir: expect_env_var("NUN_LOG_DIR", "/tmp/data/nun_db/log"),
Copy link
Owner

Choose a reason for hiding this comment

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

This is not used anywhere, are you planning to add this to some other part of the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah will hunt down where they should be in the codes and add more for the snapshot thing later.


#[test]
fn run_mode_should_get_empty_but_debug_mode_got_value() {
let config = get_configuration();
Copy link
Owner

Choose a reason for hiding this comment

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

interesting approach with the test...

ws_address: &str,
http_address: &str,
tcp_address: &str,
Copy link
Owner

Choose a reason for hiding this comment

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

Why change the order???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bcos 3012,3013,3014...

src/lib/configuration.rs Outdated Show resolved Hide resolved
createdb.sh Outdated Show resolved Hide resolved
debug.sh Outdated Show resolved Hide resolved
Some(dir_name) => dir_name.into_string().unwrap(),
None => DIR_NAME.to_string(),
}
get_configuration().nun_dbs_dir
Copy link
Owner

Choose a reason for hiding this comment

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

Here we are building the config object with several props and keys to use one... we should find a better way to build it only once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe can try lazy_static crate... later i try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now is using lazy_static @mateusfreira can take a look

src/main.rs Outdated
init_logger();
let config = configuration::get_configuration();

if config.nun_user.as_str() == "" || config.nun_pwd == "" {
Copy link
Owner

Choose a reason for hiding this comment

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

This is problematic because, because user and PWD can also be provided via command line parameter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so the priority is command line then env... will put that into a startup_condition_check function

src/lib/configuration.rs Outdated Show resolved Hide resolved
@mateusfreira mateusfreira marked this pull request as draft February 8, 2022 11:15
@mateusfreira mateusfreira marked this pull request as ready for review February 10, 2022 10:00
Copy link
Owner

@mateusfreira mateusfreira 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 the update, minor changes needed to merge

.vscode/launch.json Outdated Show resolved Hide resolved
src/lib/configuration.rs Outdated Show resolved Hide resolved
@mateusfreira mateusfreira changed the base branch from master to contralize-configuration February 10, 2022 12:34
@mateusfreira mateusfreira merged commit 414ffca into mateusfreira:contralize-configuration Feb 10, 2022
@mateusfreira
Copy link
Owner

mateusfreira commented Feb 10, 2022

Thanks a lot for the contribution @thienpow. This PR will help a lot to standardize and document all our configurations; this will be merged to master with a few code changes in the PR #64 as soon as the build gets green

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.

None yet

2 participants