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

Andrewmwells/protobufs #1277

Merged
merged 45 commits into from
Oct 23, 2024
Merged

Andrewmwells/protobufs #1277

merged 45 commits into from
Oct 23, 2024

Conversation

andrewmwells-amazon
Copy link
Contributor

@andrewmwells-amazon andrewmwells-amazon commented Oct 16, 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 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).

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

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.

Brandon-Rozek and others added 30 commits June 24, 2024 23:18
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Brandon Rozek <[email protected]>
Signed-off-by: Andrew Wells <[email protected]>
Signed-off-by: Andrew Wells <[email protected]>
Signed-off-by: Andrew Wells <[email protected]>
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.

Looks like reasonable code and well tested, happy to accept it under an experimental flag

cedar-policy-core/Cargo.toml Outdated Show resolved Hide resolved
cedar-policy-core/Cargo.toml Outdated Show resolved Hide resolved
cedar-policy-core/Cargo.toml Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/entity.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/entity.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/entity.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/entity.rs Outdated Show resolved Hide resolved
cedar-policy-validator/Cargo.toml Outdated Show resolved Hide resolved
@andrewmwells-amazon andrewmwells-amazon marked this pull request as ready for review October 18, 2024 22:09
@andrewmwells-amazon
Copy link
Contributor Author

Signed-off-by: Andrew Wells <[email protected]>
Copy link
Contributor

@shaobo-he-aws shaobo-he-aws left a comment

Choose a reason for hiding this comment

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

LGTM.

cedar-policy-core/schema/AST.proto Outdated Show resolved Hide resolved
}

#[cfg(feature = "protobufs")]
impl From<&Expr> for proto::Expr {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change this to TryFrom to avoid unimplemented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The obvious way to do this would require exposing a protobuf decode error upstream. I need to think more about the right approach, but I think this is ok for an experimental feature.

@@ -840,6 +840,307 @@ impl std::fmt::Display for Unknown {
}
}

#[cfg(feature = "protobufs")]
impl From<&proto::Expr> for Expr {
// PANIC SAFETY: experimental feature
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe this comment should be more specific?

cedar-policy-validator/schema/Validator.proto Outdated Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
cedar-policy-core/build.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast.rs Show resolved Hide resolved
cedar-policy-core/src/ast/entity.rs Outdated Show resolved Hide resolved
cedar-policy-core/src/ast/expr.rs Show resolved Hide resolved
cedar-policy-core/src/ast/literal.rs Outdated Show resolved Hide resolved
cedar-policy/CHANGELOG.md Outdated Show resolved Hide resolved
@andrewmwells-amazon andrewmwells-amazon merged commit 6b16db5 into main Oct 23, 2024
16 of 19 checks passed
@andrewmwells-amazon andrewmwells-amazon deleted the andrewmwells/protobufs branch October 23, 2024 17:41
shaobo-he-aws added a commit that referenced this pull request Nov 5, 2024
This reverts commit 6b16db5.

Signed-off-by: Shaobo He <[email protected]>
@adpaco-aws adpaco-aws mentioned this pull request Nov 11, 2024
4 tasks
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.

5 participants