Skip to content

update for cedar#726 (version numbers) - #106

Merged
shaobo-he-aws merged 2 commits into
mainfrom
cdisselkoen/update-for-cedar-726
Mar 15, 2024
Merged

update for cedar#726 (version numbers)#106
shaobo-he-aws merged 2 commits into
mainfrom
cdisselkoen/update-for-cedar-726

Conversation

@cdisselkoen

Copy link
Copy Markdown
Contributor

Issue #, if available:

cedar-policy/cedar#726

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

Signed-off-by: Craig Disselkoen <cdiss@amazon.com>
Signed-off-by: Craig Disselkoen <cdiss@amazon.com>

@khietakhieta left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Approving as-is, but left a couple small comments.

Comment on lines +13 to +14
thiserror = "1.0.49"
itertools = "0.11.0"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For consistency with our other repos

Suggested change
thiserror = "1.0.49"
itertools = "0.11.0"
thiserror = "1.0"
itertools = "0.12"

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Was about to fix this but @shaobo-he-aws merged the PR before I had a chance. Feel free to make a followup

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow up PR: #107

version = "4.0.0"

[dependencies]
cedar-policy = { version = "4.0.0", path = "../../cedar/cedar-policy" }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A little weird to require cedar-policy to be cloned locally -- switch to using the git trick from https://github.com/cedar-policy/cedar-examples/blob/release/3.1.x/tinytodo/Cargo.toml?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

This is pre-existing, let's make an issue to fix this in a different PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Follow up PR: #107

@shaobo-he-aws
shaobo-he-aws merged commit 83b918e into mainMar 15, 2024
@shaobo-he-aws
shaobo-he-aws deleted the cdisselkoen/update-for-cedar-726 branch March 15, 2024 17:27
Sign up for freeto 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

@cdisselkoen@khieta@aaronjeline@shaobo-he-aws