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

move protobuf code to cedar-policy #1452

Merged
merged 12 commits into from
Feb 11, 2025
Merged

Conversation

cdisselkoen
Copy link
Contributor

Description of changes

Moves all protobuf-related code out of cedar_policy_core and cedar_policy_validator and into a new (public) submodule of cedar_policy. This should resolve the build issues which resulted from packages having to depend on .proto files in other crates.

This necessitated various changes to the internal APIs of cedar_policy_core and cedar_policy_validator. If someone feels strongly, I can split out those changes into a separate PR.

No changes to the public API, other than that the actual Rust structures compiled from protobuf are now public in cedar_policy, behind the protobufs experimental flag.

Issue #, if available

Checklist for requesting a review

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

  • A change "invisible" to users (e.g., documentation, changes to "internal" crates like cedar-policy-core, cedar-validator, etc.)

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

  • 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):

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

I confirm that docs.cedarpolicy.com (choose one, and delete the other options):

  • Does not require updates because my change does not impact the Cedar language specification.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen
Copy link
Contributor Author

I'm confused by the current build failure (which happens while trying to compile code that was generated by prost). Rust-analyzer seems to find and compile the files correctly, but cargo check does not.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen
Copy link
Contributor Author

Build is now passing. CI just reporting that DRT build fails, I will make a separate PR to deal with that (but not merge this until the DRT PR is ready). This PR is ready for review.

cedar-policy/src/proto/validator.rs Show resolved Hide resolved
cedar-policy-core/src/entities.rs Outdated Show resolved Hide resolved
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Copy link
Contributor

@adpaco-aws adpaco-aws left a comment

Choose a reason for hiding this comment

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

Thanks!

…public

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
@cdisselkoen
Copy link
Contributor Author

Ready for review -- cedar-policy/cedar-spec#532 will address the downstream CI failure for DRT.

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
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.

looks reasonable

@cdisselkoen cdisselkoen merged commit 3a0d233 into main Feb 11, 2025
18 of 19 checks passed
@cdisselkoen cdisselkoen deleted the cdisselkoen/protobuf-public branch February 11, 2025 14:29
cdisselkoen added a commit that referenced this pull request Feb 11, 2025
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cdisselkoen added a commit that referenced this pull request Feb 11, 2025
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cdisselkoen added a commit that referenced this pull request Feb 11, 2025
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
cdisselkoen added a commit that referenced this pull request Feb 11, 2025
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
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.

3 participants