-
Notifications
You must be signed in to change notification settings - Fork 147
Change minimize to a boolean flag with an option minimize-length argument #270
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
Conversation
@kmcquade This is what we discussed in the other PR. Let me know what you think. I'm going to give some though on how this might be done without breaking backward compatibility. |
I've been playing with terraform and I found a way to remove the need for the extraneous --no-minimize by constructing the program[] list with a condition and concat(). I'm still researching if there's any way to maintain backward compatibility with --minimize {value} |
@dgubitosi I am okay with breaking the backward compatibility here. Can you change the version in policy_sentry/bin/cli.py to 0.10.0? We'll make sure it shows up in the release notes with breaking changes |
@kmcquade We can also label the PR with minor label and the release notes will be updated policy_sentry/.github/release-drafter.yml Line 27 in 54066aa
|
@reetasingh - @dgubitosi will still need to update the |
|
I managed to maintain backward compatibility. policy_sentry write-policy --input-file foo.yaml => does not minimize And I removed the extraneous --no-minimize flag since I can work around it in terraform. |
Fantastic! @dgubitosi if this is in the final state, let me know and we can merge |
Hmm. I've been doing some more experimentation with click. I thought is_eager on the flag helps enforce that the argument had to follow it, but that's not the case. It works the same without it, and in reality I'm just abusing an argument to mimic the previous command line behavior. For example, these two commands are identical: policy_sentry write-policy --input-file foo.yaml --minimize 5 Similarly these two commands are identical and valid as side-effect of my argument abuse: policy_sentry write-policy --input-file foo.yaml 5 Click captures the numeric argument where ever it is on the command and regardless if the --minimize flag is present. However, the argument value has no purpose unless its accompanied by the --minimize flag. The code ignores it when the flag is not present. Do you think this is an issue? |
Hmmm. I'm not sure about this one. It's kind of like a bug by design lol. @reetasingh what do you think about this one? I could use your opinion. |
I didn't like the argument abuse so I found the proper solution. As you can see, the code is more complicated as it required using custom classes and a custom parser to overcome this limitation of click. The solution uses a "hidden" option, --minimize_length, and a special attribute attached to --minimize that is used in the parser. The parser looks for all options with the special attribute so that they can be processed specially. In this case, when the parser encounters --minimize on the arguments list, it checks to see if there's an argument following it. If so, it moves that argument into the hidden --minimize_length option. This ensures that the command line is in the correct order. All of these are valid and identical: These are now invalid as the 5 is a stray argument: Technically, these are also valid as the hidden option can be supplied on the command line: And this leads in a side effect as this is also valid: This gets expanded by the custom parser into this: When this is encountered, the value of the last instance of --minimize_length is used |
I'll take a look at addressing those pylint checks. |
Fantastic! Sure. When those are fixed I'll go ahead and merge. Really appreciate the contribution here. |
I incremented the version in cli.py to 0.10.0 as discussed. I will start on a PR for terraform-aws-policy-sentry so it conforms to this new version. |
What does this PR do?
Changes the --minimize option to a boolean flag. The default is --no-minimize and does not need to be supplied on the command line. When --minimize is supplied, the default character length is zero. An optional --minimize-length option can be used to override and only accepts integers >= 0.
What gif best describes this PR or how it makes you feel?
N/A
Completion checklist