Skip to content

Conversation

shaobo-he-aws
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws commented Mar 14, 2024

Description of changes

Issue #, if available

Checklist for requesting a review

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

  • A backwards-compatible change requiring a minor version bump to cedar-policy (e.g., addition of a new API).

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 formal model or DRT infrastructure.

@john-h-kastner-aws
Copy link
Contributor

Is this fixing #681?

@shaobo-he-aws
Copy link
Contributor Author

Is this fixing #681?

Yes, thanks for pointing it out.

@john-h-kastner-aws
Copy link
Contributor

Do we need to make the same change for entity type attributes?

Copy link

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

LGTM

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.

Lgtm. Left one small comment about test cases.

@shaobo-he-aws
Copy link
Contributor Author

shaobo-he-aws commented Mar 15, 2024

Do we need to make the same change for entity type attributes?

Yes, will do it in another PR. Actually, we don't need the same change because entity type attributes can be arbitrary types, including common types.

Signed-off-by: Shaobo He <[email protected]>
@shaobo-he-aws shaobo-he-aws merged commit d6d5e98 into main Mar 15, 2024
@shaobo-he-aws shaobo-he-aws deleted the fix/shaobo/human-schema-context branch March 15, 2024 17:43
shaobo-he-aws added a commit that referenced this pull request Apr 11, 2024
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