Clap4 #336
Clap4 #336
Conversation
…step, only ran every night
joshfried-aws
requested review from
razcloud,
Brianwithay21,
wkexiang,
Neel-Renavikar,
reza-aws and
akshayrane
| @@ -976,7 +1043,7 @@ dependencies = [ | |||
| "pin-project-lite", | |||
| "socket2", | |||
| "tokio-macros", | |||
| "windows-sys", | |||
| "windows-sys 0.42.0", | |||
There was a problem hiding this comment.
Why is this version different from the one in its own config, which is 0.45.0?
There was a problem hiding this comment.
im assuming it has to do with a transitive dependency for clap?
| Testing Guard File resources/test-command/dir/s3_bucket_logging_enabled.guard | ||
| Test Case #1 | ||
| Name: Empty, SKIP | ||
| `- File(, Status=SKIP)[Context=File(rules=1)] |
There was a problem hiding this comment.
Is the first arg expected to be blank in File(,? It could be but we might want to double check.
There was a problem hiding this comment.
this is the output i got when running guard 2.1.3, i used that output to verify
| .long(PRINT_JSON.0) | ||
| .short(PRINT_JSON.1) | ||
| .required(false) | ||
| .action(ArgAction::SetTrue) |
There was a problem hiding this comment.
Do we want really want to remove .required(false) here? That will make this flag mandatory no?
There was a problem hiding this comment.
since its a flag, i.e. its default value is false we dont need it to be required.
from the docs
Pro tip: Flags (i.e. not positional, or arguments that take values) shouldn’t be required by default. This is because if a flag were to be required, it should simply be implied. No additional information is required from user. Flags by their very nature are simply boolean on/off switches. The only time a user should be required to use a flag is if the operation is destructive in nature, and the user is essentially proving to you, “Yes, I know what I’m doing.”
| Some(file) => Box::new(std::io::BufReader::new(File::open(file)?)), | ||
| None => Box::new(reader), | ||
| }; | ||
|
|
||
| let yaml = !app.is_present(PRINT_JSON.0); | ||
| let yaml = !app.get_flag(PRINT_JSON.0); |
There was a problem hiding this comment.
Wait, we do have a print-yaml flag but we aren't using that? We are negating print-json istead?
There was a problem hiding this comment.
Yeah seems like thats the way they do it. Would you rather I just remove the flag alltogether ?
| @@ -56,6 +54,15 @@ pub(crate) enum Type { | |||
| Generic, | |||
| } | |||
|
|
|||
| impl From<&str> for Type { | |||
There was a problem hiding this comment.
Need to understand why is this additional snippet required? Is it because older version of Clap seamlessly converted strings to enums but newer one doesn't?
There was a problem hiding this comment.
no it didnt, it still treated it as string and then matched on it inside the execute function. The logic is the same, but this is just a more idiomatic way to handle conversions in rust
| .arg(Arg::new(DATA.0) | ||
| .long(DATA.0) | ||
| .short(DATA.1) | ||
| .num_args(0..) |
There was a problem hiding this comment.
AFAIU the documentation recommends against this approach and suggests to use either of the two ArgAction::Append or num_args. Is that not the case? Have you verified by running the command with multiple data and rules args and it worked?
A common mistake is to define an option which allows multiple values and a positional argument.
The problem is clap doesn’t know when to stop parsing values for “file”.
A solution for the example above is to limit how many values with a maximum, or specific number, or to say ArgAction::Append is ok, but multiple values are not.
There was a problem hiding this comment.
Is that not the case? Have you verified by running the command with multiple data and rules args and it worked?
we have e2e tests that already do this for us?
I added num_args because the tests we had covering this exact scenario failed
There was a problem hiding this comment.
furthermore without num_args(0..) the only way clap would allow us to have multiple values would be to either specify a delimiter or to keep passing either the rules/data/input-params flag everytime we add another value.
This approach provides the same exact behaviour to the customer
| .short(TYPE.1) | ||
| .action(ArgAction::Set) | ||
| .required(false) | ||
| .help("Specify the type of data file used for improved messaging - ex: CFNTemplate")) |
There was a problem hiding this comment.
We have moved possible_values(["CFNTemplate"]) part to the string to enum processing snippet haven't we?
Shouldn't we have a similar .value_parser(TYPE_BLAH_MEH) here?
There was a problem hiding this comment.
I dont think thats the correct behaviour since if we had something like
TYPES_VALUES: [&str: 1] = ["CFNTemplate"]
we would error out when any other value was passed, instead if any other value is passed we use the generic handler
| .arg(Arg::new(SHOW_CLAUSE_FAILURES.0) | ||
| .long(SHOW_CLAUSE_FAILURES.0) | ||
| .short(SHOW_CLAUSE_FAILURES.1) | ||
| .action(ArgAction::SetTrue) |
There was a problem hiding this comment.
We are missing required() here again with few of the args, from the documentation I couldn't tell if absence of this implies that this is a mandatory argument or not. So just pointing it out, in case it does.
There was a problem hiding this comment.
see previous comment about flags
| None => OutputFormatType::SingleLineSummary, | ||
| }; | ||
|
|
||
| let summary_type: BitFlags<SummaryType> = | ||
| app.values_of(SHOW_SUMMARY.0) | ||
| app.get_many::<String>(SHOW_SUMMARY.0) |
There was a problem hiding this comment.
SHOW_SUMMARY does not have multiple args, right? get_many is only supposed to be used when we are using the same option multiple times to collect all values, correct? Like for RULES and DATA it makes sense.
We should use get_one() here no?
There was a problem hiding this comment.
Correct me if Im wrong but this was the previous show-summary definition
.arg(Arg::with_name(SHOW_SUMMARY.0).long(SHOW_SUMMARY.0).short(SHOW_SUMMARY.1).takes_value(true).use_delimiter(true).multiple(true) .possible_values(["none", "all", "pass", "fail", "skip"])
this multiple(true) implies that its a vector of String values.
guard/tests/utils.rs
Outdated
| @@ -66,7 +64,7 @@ pub trait CommandTestRunner { | |||
|
|
|||
| fn run(&self, mut writer: &mut Writer, mut reader: &mut Reader) -> i32 { | |||
| let test_app_name = String::from("cfn-guard-test"); | |||
| let mut app = App::new(&test_app_name); | |||
| let mut app = clap::Command::new("cfn-guard-test"); | |||
There was a problem hiding this comment.
We could still use the variable here? test_app_name
There was a problem hiding this comment.
not sure why i changed that, good catch
* Improve handling of function references for test command (#331) * improving handling for tagged values for test command * removing shorthand functions from known issues * PR to add Thiserror to cfn-guard (#329) * PR to add thiserror to guard * cleanup * clippy lints * printing errs to stderr instead of stdout * merge conflicts * rustfmt * cleaning up code * Redirected verbose output from stdout to custom writer and added unit… (#332) * Redirected verbose output from stdout to custom writer and added unit tests * Updated one more occurrence & removed new line from 2 places --------- Co-authored-by: Akshay Rane <raneaks@amazon.com> * Addit cargo-audit to CI + bump up clap to 3.0 (#330) * init commit, bumped up clap to 3.0 * cargo audit for CI * reverting last commit + changing cargo-audit to not be part of build step, only ran every night * changing cronjob to run at noon instead of midnight * fixing error in cronjob scheduling * Implemented custom reader, increasing test coverage for validate command. (#334) * init commit for reader + more unit tests for validate * init commit for reader + more unit tests for validate * temp * temp2 * rebasing * clippy lints for validate * more tests for validate * adding thiserror license to attributions * removing useless code * renaming tests * fix failing test * fixes as per comments * attribution update * moved get_reader fn to utils * Update CONTRIBUTING.md (#335) * Clap4 (#336) * init commit, bumped up clap to 3.0 * cargo audit for CI * reverting last commit + changing cargo-audit to not be part of build step, only ran every night * init commit bumping up to clap4 * cleaning some code up * temp commit * rebasing + fixing small bug * cleanup * adding help messages * adding more tests for test_command * more tests for prev engine * improving code style * typo * fixed unecessary match * cleaning up test command * fixes as per comments * fixes as per comments * fixed failing build * Added integration tests against aws-guard-rules-registry on Ubuntu (#337) * Added GitHub action for integration test with rules registry * Changed branch name for testing * Changed ref tag * Changed repo param * Removed tags temporarily * Added temp in-place replacement and main branch as ref * Corrected the sequence of commands * Added parse-tree integration tests * Changed dir and limited build to one crate * Added names to each job * Added logic to allow capturing of exit codes * Testing exit code script * Added if as part of error handling * Added statements for test command * Version before changing branch * Changed branches * Bug fix for output arg being pulled before we enter command context * Workaround to skip comments only files * Added test remote branch back * Corrected the condition * Made SKIPPED_RULE_COUNT a variable * Updated display messages * Removed tabs * Updated branches * Updated code to extract OUTPUT arg from subcommand instead of the app --------- Co-authored-by: Akshay Rane <raneaks@amazon.com> * Update check-tags-present.guard (#313) replaced "is" with "if". Also. can you provide more detailed explanation for this guard file * Adding structured evaluator (#339) * init commit for reader + more unit tests for validate * implemented structured reporter + some small refactorings * clippy lints * adding structured reporter for payload code path * adding test for structured payload * cleanup * adding previoius-engine as a conflict with structured * adding fix for recursive serialization issue * adding test for structured yaml * Added deprecated short flag for print-json in parse-tree (#345) * Added deprecated short flag for print-json in parse-tree * Formatting changes * Added docs --------- Co-authored-by: Akshay Rane <raneaks@amazon.com> --------- Co-authored-by: Akshay Rane <aks.rane@gmail.com> Co-authored-by: Akshay Rane <raneaks@amazon.com> Co-authored-by: swiercek <111157886+swiercek@users.noreply.github.com> Co-authored-by: Aishwarya4400 <50081627+Aishwarya4400@users.noreply.github.com>
Issue #, if available:
Description of changes:
This PR addresses the changes required to bump up to clap v4. We also added more test for testing whole directories in the test command. This covers verbose output for both previous engine, and the new engine.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license