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

Fix simple sub-command option consistency #406

Conversation

svanderbleek
Copy link
Contributor

@svanderbleek svanderbleek commented Nov 7, 2023

Description of changes

I made the "easy" CLI argument consistency changes. I changed principal to -l and cleaned up other places where there was inconsistency like schema short -s not being everywhere and changing template-linked -t to -k because it would conflict if the short was added to the other use.

Harder to determine questions are

  • if positional argument for policy should be removed from format and changed to use the option only (or support both somehow)
  • if standard input reading of policy file should be added everywhere

Issue #403

Checklist for requesting a review

I'm not sure how to fill this in, the changes could be considered breaking if someone coded against the CLI. There was no way to handle -p conflict without it being breaking.

The change in this PR is (choose one, and delete the other options):

  • A breaking change requiring a major version bump to cedar-policy (e.g., changes to the signature of an existing API).
  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).
  • A bug fix or other functionality change requiring a patch to cedar-policy.
  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)
  • A change (breaking or otherwise) that only impacts unreleased or experimental code.

I confirm that this PR (choose one, and delete the other options):

  • Updates the "Unreleased" section of the CHANGELOG with a description of my change (required for major/minor version bumps).
  • Does not update the CHANGELOG because my change does not significantly impact released code.

I confirm that cedar-spec (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar Dafny model or DRT infrastructure.
  • Requires updates, and I have made / will make these updates myself. (Please include in your description a timeline or link to the relevant PR in cedar-spec, and how you have tested that your updates are correct.)
  • Requires updates, but I do not plan to make them in the near future. (Make sure that your changes are hidden behind a feature flag to mark them as experimental.)
  • I'm not sure how my change impacts cedar-spec. (Post your PR anyways, and we'll discuss in the comments.)

Disclaimer

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Contributor

@cdisselkoen cdisselkoen 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 PR! This change makes sense to me. And we can ship breaking behavior changes in the upcoming 3.0 release, so no worries there.

Copy link
Contributor

@aaronjeline aaronjeline left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

@cdisselkoen
Copy link
Contributor

I took the liberty of adding a small changelog update on the PR branch. Let me know if there's anything wrong with it

@cdisselkoen cdisselkoen merged commit e12bf86 into cedar-policy:main Nov 7, 2023
7 checks passed
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.

4 participants