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

obfuscation is now turnoffable #20

Merged
merged 4 commits into from
May 22, 2023
Merged

obfuscation is now turnoffable #20

merged 4 commits into from
May 22, 2023

Conversation

enola-dkfz
Copy link
Member

No description provided.

@enola-dkfz enola-dkfz requested a review from TKussel May 19, 2023 13:03
```bash
RETRY_COUNT = "32"
```

The default maximum number of retries for beam and blaze healthchecks is 32.

To disable the count obfuscation, set the env. variable `DO_NOT_OBFUSCATE`.
Copy link
Member

Choose a reason for hiding this comment

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

Please give values for on / off.

Consider using a positive variable (negations are confusing)

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't work with boolean.
clap-rs/clap#1649
Do you want it to be a string?

Copy link
Member

Choose a reason for hiding this comment

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

Surprising but then I'm fine with it

@@ -42,6 +42,10 @@ struct CliArgs {
#[clap(long, env, value_parser)]
blaze_url: Uri,

/// Should the results not be obfuscated
#[clap(long, env, value_parser)]
Copy link
Member

Choose a reason for hiding this comment

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

Don't you want to set default value == false (meaning "yes, obfuscate?)

negations are confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

When the parameter is not present, it is false, no default needed

src/main.rs Outdated
true => cql_result
};

dbg!(cql_result_new.clone());
Copy link
Member

Choose a reason for hiding this comment

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

Forgot to remove?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed

Copy link
Member

@TKussel TKussel left a comment

Choose a reason for hiding this comment

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

With all concerns of @lablans addressed, this looks fine.

@enola-dkfz enola-dkfz merged commit 6b68dda into develop May 22, 2023
@enola-dkfz enola-dkfz deleted the feature/turnoffable branch May 22, 2023 16:37
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