Skip to content

fix(policy/enforcement): update README for policy 01#416

Merged
ndr-brt merged 5 commits into
eclipse-edc:mainfrom
ortegi:main
Jun 23, 2025
Merged

fix(policy/enforcement): update README for policy 01#416
ndr-brt merged 5 commits into
eclipse-edc:mainfrom
ortegi:main

Conversation

@ortegi
Copy link
Copy Markdown
Contributor

@ortegi ortegi commented Jun 13, 2025

What this PR changes/adds

This PR updates the policy-01-policy-enforcement/README.md file to fix and clarify the instructions for running and testing the provided code.

Context

While going through the README to test the code, I noticed that some of the final instructions (Bind the constraint to the cataloging scope) didn’t work as expected. After a bit of trial and error, I found that a few parts were either outdated or incorrect.

This is what i found:

  1. The CATALOGING_SCOPE doesn’t seem to exist. The actual constant is CATALOG_SCOPE, and the correct context class to use with policyEngine.registerFunction is CatalogPolicyContext.class.

  2. The import statement for the correct constant (CATALOG_SCOPE) was missing.

  3. A code change in LocationConstraintFunction.java is also needed. This wasn't mentioned in the README, but in order for everything to work, the LocationConstraintFunction class needs to be updated to use CatalogPolicyContext instead of ContractNegotiationPolicyContext.

Who will sponsor this feature?

ndr-brt

Linked Issue(s)

Closes #245

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

@ortegi ortegi requested a review from ronjaquensel as a code owner June 13, 2025 14:24
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

'We are always happy to welcome new contributors ❤️ To make things easier for everyone, please - make sure to follow our contribution guidelines, - check if you have already signed the ECA, and - relate this pull request to an existing issue or discussion.'

@ndr-brt ndr-brt self-requested a review June 18, 2025 08:01
Comment thread policy/policy-01-policy-enforcement/README.md Outdated
Comment on lines +405 to +411
import org.eclipse.edc.connector.controlplane.catalog.spi.policy.CatalogPolicyContext;
import static org.eclipse.edc.connector.controlplane.catalog.spi.policy.CatalogPolicyContext.CATALOG_SCOPE;

ruleBindingRegistry.bind(LOCATION_CONSTRAINT_KEY, CATALOG_SCOPE);
policyEngine.registerFunction(CatalogPolicyContext.class, Permission.class, LOCATION_CONSTRAINT_KEY, new LocationConstraintFunction(monitor));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure about adding these imports, nowadays IDEs are pretty good in doing that automatically.

At least please add some points (...) in the middle to show that these lines needs to go in different part of the class

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In my case (using IntelliJ IDEA 2025.1.2 Community Edition), the import wasn’t added automatically, so I included it manually.

I also already added some points (...) to show that these lines belong in different parts of the class.

That said, it's completely up to you whether to keep this part :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

In my case (using IntelliJ IDEA 2025.1.2 Community Edition), the import wasn’t added automatically, so I included it manually.

maybe not 100% automatically, but with a simple Alt+Enter it should suggest the correct options, to me that's pretty automatic

I also already added some points (...) to show that these lines belong in different parts of the class.

That said, it's completely up to you whether to keep this part :)

What do you mean?

Copy link
Copy Markdown
Contributor Author

@ortegi ortegi Jun 20, 2025

Choose a reason for hiding this comment

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

Okay, i already deleted the unnecessary imports.

Comment thread policy/policy-01-policy-enforcement/README.md Outdated
@ndr-brt
Copy link
Copy Markdown
Member

ndr-brt commented Jun 20, 2025

@ortegi you need to sign the ECA to being able to contribute (see the related broken check), also to rename the PR to follow the project convention.

@ndr-brt ndr-brt added the documentation Improvements or additions to documentation label Jun 20, 2025
ortegi and others added 4 commits June 20, 2025 19:27
@ortegi ortegi changed the title fix policy-01-policy-enforcement/README.md fix(policy/enforcement): update README for policy 01 Jun 20, 2025
Copy link
Copy Markdown
Member

@ndr-brt ndr-brt left a comment

Choose a reason for hiding this comment

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

LGTM

@ndr-brt ndr-brt merged commit 44f6729 into eclipse-edc:main Jun 23, 2025
3 checks passed
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.

policy-01-policy-enforcement

2 participants