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(conn): elaborate on CX conventions on Policies #792

Merged
merged 16 commits into from
May 23, 2024

Conversation

arnoweiss
Copy link
Contributor

@arnoweiss arnoweiss commented Mar 25, 2024

Description

This PR adds a new section to the Connector Kit representing the result of the Data Sovereignty Task Force including some implementation Tips for Data Providers and Enablement Service Providers building Connectors.

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for committer review:

@arnoweiss arnoweiss marked this pull request as ready for review March 28, 2024 15:11
mhellmeier
mhellmeier previously approved these changes Mar 28, 2024

Like all payloads that get passed between connectors, ODRL is an RDF-based description language that is on the wire
serialized as JSON-LD. JSON-LD is namespace-aware JSON with a couple of twists that one should be aware of when working
with it (like "Structures may be semantically equivalent even though, schematically, they are clearly not"). ODRL
Copy link
Contributor

Choose a reason for hiding this comment

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

even though correct, I think this causes more confusion for the reader then it helps (without an example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm trying to encourage everyone reading to familiarize themselves with the concept of json-ld. Not doing so will lead to even more confusion further down the road.

`odrl:Constraint`.

If, for example, the Consumer tries to negotiate for an Offer that is extended only to interested
parties from civil society (like an NGO), simply pretending to be an NGO shouldn't be enough. It has to be verified and
Copy link
Contributor

Choose a reason for hiding this comment

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

Example should be closer to Catena-X, e.g. Dismantler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As explained - I think there's value in explaining the concepts in the abstract and then narrowing it down to its application in Catena-X

Copy link
Contributor

Choose a reason for hiding this comment

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

abstract makes sense, yes. but different use case imho not.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same, I would go for the MembershipCredential

"id": "1f36af58-0fc0-4b24-9b1c-e37d59668089",
"type": [
"VerifiableCredential",
"NonGovernmentalOrganizationCredential"
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, this is something we don't have in CX. We should not use this in our examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should explain the generic concept on one page (working-with-policies.md) and their application on another (policies-in-catena.md).

Copy link
Contributor

@matgnt matgnt left a comment

Choose a reason for hiding this comment

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

One minor comment added.

I don't like the examples, because I think they are not 'abstract' but just too far away from our use cases, but other than that, LGTM.

### Chaining Constraints

If a Policy is supposed to hold multiple constraints, Data Providers may chain them via a logical AND. This can be
achieved via an `odrl:and` object encapsulating multiple other `odrl:Constraint`s or entering a list of them
Copy link
Contributor

Choose a reason for hiding this comment

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

Formally correct you could say something like:

"...Rule classes constraint property may hold a Constraint or a LogicalConstraint that itself holds - in case of a logical AND - an odrl:and property with a list of Cosntraints."

but this is really a minor comment...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd prefer to leave it this way - especially since it also opens the option to

{
  "action": "use",
  "constraint": [
    {
      "leftOperand": "cx-policy:FrameworkAgreement",
      "operator": "eq",
      "rightOperand": "traceability:1.0"
    },
    {
      "leftOperand": "cx-policy:ContractReference",
      "operator": "eq",
      "rightOperand": "x12345"
    },
    {
      "leftOperand": "cx-policy:UsagePurpose",
      "operator": "eq",
      "rightOperand": "cx.core.industrycore:1"
    }
  ]
}

Copy link
Contributor

Choose a reason for hiding this comment

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

which is basically also ODRL and there is no way to not allow this - to my understanding.... and btw: your example would have been my preferred solution / documentation examples.
Even though in my text above, I didn't mention this. I just described the LogicalConstraint that we are using - as kind of mandatory - which it isn't.
but as I said, this comment was a minor one. just ignore it :-)

Copy link
Contributor

@matgnt matgnt left a comment

Choose a reason for hiding this comment

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

LGTM

mhellmeier
mhellmeier previously approved these changes Apr 23, 2024
Copy link
Member

@mhellmeier mhellmeier left a comment

Choose a reason for hiding this comment

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

Thanks for fixing and implementing our review comments!

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

Got smaller findings. Great contribution, it will help people to better understand how contracts are defined and evaluated!

`odrl:Constraint`.

If, for example, the Consumer tries to negotiate for an Offer that is extended only to interested
parties from civil society (like an NGO), simply pretending to be an NGO shouldn't be enough. It has to be verified and
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, I would go for the MembershipCredential

Copy link
Contributor

@tom-rm-meyer-ISST tom-rm-meyer-ISST left a comment

Choose a reason for hiding this comment

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

LGTM

@arnoweiss arnoweiss requested a review from jimmarino May 14, 2024 15:59
Copy link

@HFocken HFocken left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution - much appreciated - a few formal comments to fix, then ready to go.

@HFocken
Copy link

HFocken commented May 15, 2024

Overarching question/comment: could you please add / adapt the Changelog? It was not modified for this PR.

@arnoweiss
Copy link
Contributor Author

The build is failing due to an error response from clearlydefined, will try again tomorrow morning.

@mhellmeier
Copy link
Member

The build is failing due to an error response from clearlydefined, will try again tomorrow morning.

I fixed it in the parallel PR (#875). After it is merged, you just need to merge or rebase main.

Copy link

@HFocken HFocken left a comment

Choose a reason for hiding this comment

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

Thanks for addressing all change requests! Good to go from my end!

@arnoweiss arnoweiss requested a review from mhellmeier May 23, 2024 09:56
Copy link
Member

@mhellmeier mhellmeier left a comment

Choose a reason for hiding this comment

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

Thanks for implementing my review comments!

@matgnt
Copy link
Contributor

matgnt commented May 23, 2024

Only the very recent change in Credential name <-> policy mapping would be left imo.
See:
catenax-eV/cx-odrl-profile#7
Or should we create a separate PR afterwards?

@arnoweiss
Copy link
Contributor Author

Only the very recent change in Credential name <-> policy mapping would be left imo. See: catenax-eV/cx-odrl-profile#7 Or should we create a separate PR afterwards?

let's create a separate PR when the one you linked is merged.

@arnoweiss arnoweiss merged commit b6e6292 into eclipse-tractusx:main May 23, 2024
5 checks passed
@arnoweiss arnoweiss deleted the docs/connector/prep-r2405 branch May 23, 2024 14:42
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.

None yet

6 participants