Skip to content

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Allows annotation keys to be Cedar reserved words. Tests that this works both for human-format and JSON-format (EST) policies. Fixes #623.

Issue #, if available

#623

Checklist for requesting a review

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

  • A bug fix or other functionality change requiring a patch to cedar-policy.

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

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.

(is that right?)

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

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

This seems reasonable, but introduces another flavor of inconsistency in how we handle reserved identifiers.

Eliminating reserved identifiers as much as possible in other locations could be a good path towards consistency here. Probably worth discussing further.

Copy link
Contributor

@khieta khieta left a comment

Choose a reason for hiding this comment

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

Looks fine to me. I don't see a problem with allowing reserved words to be identifiers.

Edit: Agreed with @john-h-kastner-aws's comment above that it might be nice to use AnyId in other places where we currently use Id (not sure exactly where those places are, since I haven't looked at the code too recently). File an issue?

@cdisselkoen
Copy link
Contributor Author

Opened #635 for the follow-up. Unless @john-h-kastner-aws thinks this is important to address now (before merging)?

Copy link
Contributor

@john-h-kastner-aws john-h-kastner-aws left a comment

Choose a reason for hiding this comment

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

I think we can merge this now

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.

EST allows annotation identifiers that are forbidden in human formated policies
3 participants