Skip to content

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

Merged
merged 10 commits into from
Nov 3, 2020

Conversation

dgubitosi
Copy link
Contributor

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.

  • Changes --minimize to a boolean flag. Unfortunately, this breaks compatibility.

What gif best describes this PR or how it makes you feel?

N/A

Completion checklist

@dgubitosi
Copy link
Contributor Author

@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.

@dgubitosi
Copy link
Contributor Author

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}

@kmcquade
Copy link
Collaborator

kmcquade commented Oct 27, 2020

@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

@reetasingh
Copy link
Contributor

@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

version-resolver:

@kmcquade
Copy link
Collaborator

@reetasingh - @dgubitosi will still need to update the cli.py file, and then for us to tag the release on the git side, we use the release drafter.

@kmcquade kmcquade added the minor label Oct 27, 2020
@reetasingh
Copy link
Contributor

@reetasingh - @dgubitosi will still need to update the cli.py file, and then for us to tag the release on the git side, we use the release drafter.
yes

@dgubitosi
Copy link
Contributor Author

I managed to maintain backward compatibility.

policy_sentry write-policy --input-file foo.yaml => does not minimize
policy_sentry write-policy --input-file foo.yaml --minimize => acts like a simple flag that does greedy minimize
policy_sentry write-policy --input-file foo.yaml --minimize 0 => like before will greedy minimize
policy_sentry write-policy --input-file foo.yaml --minimize 5 => like before will minimize to 5 characters

And I removed the extraneous --no-minimize flag since I can work around it in terraform.

@kmcquade
Copy link
Collaborator

Fantastic! @dgubitosi if this is in the final state, let me know and we can merge

@dgubitosi
Copy link
Contributor Author

dgubitosi commented Oct 29, 2020

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
policy_sentry write-policy 5 --minimze --input-file foo.yaml

Similarly these two commands are identical and valid as side-effect of my argument abuse:

policy_sentry write-policy --input-file foo.yaml 5
policy_sentry write-policy 5 --input-file foo.yaml

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?

@kmcquade
Copy link
Collaborator

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.

@dgubitosi
Copy link
Contributor Author

dgubitosi commented Nov 1, 2020

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:
policy_sentry write-policy --input-file foo.yaml --minimize
policy_sentry write-policy --input-file foo.yaml --minimize 5
policy_sentry write-policy --input-file foo.yaml --minimize=5
policy_sentry write-policy --minimize 5 --input-file foo.yaml
policy_sentry write-policy --minimize=5 --input-file foo.yaml

These are now invalid as the 5 is a stray argument:
policy_sentry write-policy --input-file foo.yaml 5
policy_sentry write-policy --minimize --input-file foo.yaml 5

Technically, these are also valid as the hidden option can be supplied on the command line:
policy_sentry write-policy --input-file foo.yaml --minimize --minimize_length=5
policy_sentry write-policy --input-file foo.yaml --minimize_length 5 --minimize

And this leads in a side effect as this is also valid:
policy_sentry write-policy --input-file foo.yaml --minimize_length=5 --minimize=0

This gets expanded by the custom parser into this:
policy_sentry write-policy --input-file foo.yaml --minimize_length=5 --minimize --minimize_length=0

When this is encountered, the value of the last instance of --minimize_length is used

@dgubitosi
Copy link
Contributor Author

I'll take a look at addressing those pylint checks.

@kmcquade
Copy link
Collaborator

kmcquade commented Nov 1, 2020

Fantastic! Sure. When those are fixed I'll go ahead and merge.

Really appreciate the contribution here.

@dgubitosi
Copy link
Contributor Author

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.

@dgubitosi dgubitosi marked this pull request as ready for review November 3, 2020 11:37
@reetasingh reetasingh merged commit 5a42b0f into salesforce:master Nov 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants