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

docs: typed policy engine DR #4526

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Conversation

ndr-brt
Copy link
Member

@ndr-brt ndr-brt commented Oct 3, 2024

What this PR changes/adds

Adds DR for typed policy engine refactor

Why it does that

Briefly state why the change was necessary.

Further notes

List other areas of code that have changed but are not necessarily linked to the main feature. This could be method
signature changes, package declarations, bugs that were encountered and were fixed inline, etc.

Linked Issue(s)

Part of #3511

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@ndr-brt ndr-brt added the documentation Improvements or additions to documentation label Oct 3, 2024
Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

Let's discuss this further. I don't like having two implementations of the policy engine and I also do not want to eliminate hierarchical scopes as they are useful.

- it makes no sense to have a function/validator bound to all the scopes, scopes have well defined bound, they could
share context content (e.g. `Instant now` field could be one of them), but this should happen at the `PolicyContext`
hierarchy level.
- scope hierarchy won't be a thing anymore (e.g. `scope` and `scope.child`)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is used. Let's discuss this

@ndr-brt
Copy link
Member Author

ndr-brt commented Oct 4, 2024

@jimmarino @paullatzelsperger I revisited the DR scope, now is more narrow and it will permit to keep scopes hierarchy.

@ndr-brt
Copy link
Member Author

ndr-brt commented Oct 9, 2024

@jimmarino DR is ready to review

Copy link
Contributor

@jimmarino jimmarino left a comment

Choose a reason for hiding this comment

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

👍

@ndr-brt ndr-brt merged commit 5bc8a21 into eclipse-edc:main Oct 10, 2024
3 checks passed
@ndr-brt ndr-brt deleted the 3511-DR branch October 10, 2024 07:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants