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

doc(policies): add policy description in tractusx-edc #856

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

BenediktSR
Copy link
Contributor

@BenediktSR BenediktSR commented Nov 2, 2023

WHAT

Add new policy description to tractusx-edc

WHY

There are many policy descriptions scattered around. The aim is to have a description that describes everything you need to know about policies in Catena-X. The destination of this description should be the tractusx-edc repository.

FURTHER NOTES

Closes #701

@BenediktSR BenediktSR self-assigned this Nov 2, 2023
@BenediktSR BenediktSR added the documentation Improvements or additions to documentation label Nov 2, 2023
Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label Nov 21, 2023
@BenediktSR BenediktSR marked this pull request as ready for review November 22, 2023 12:57
@BenediktSR BenediktSR changed the title doc(policies): prepare documentation for policies doc(policies): add policy description in tractusx-edc Nov 22, 2023
@github-actions github-actions bot removed the stale label Nov 23, 2023
Copy link
Contributor

@florianrusch-zf florianrusch-zf left a comment

Choose a reason for hiding this comment

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

Just did a quick syntax check 😉

docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
@matgnt
Copy link

matgnt commented Nov 30, 2023

Hi,

it seems sometimes LogicalConstraint is used in examples and sometimes not. Is there any reason behind?

docs/policies/policies.md Outdated Show resolved Hide resolved
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.

There are still a lot of differences between the example payloads in the document. I highlighted different weaknesses in my review.

I would strongly recommend going through the whole document and checking all payloads carefully, if they are consistent and aligned with the other ones presented.

docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
docs/policies/policies.md Outdated Show resolved Hide resolved
Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label Dec 13, 2023
@wolf4ood wolf4ood removed the stale label Dec 13, 2023
@stephanbcbauer
Copy link
Member

@BenediktSR Do you need support?

Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label Dec 22, 2023
Copy link
Contributor

This pull request was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this Dec 30, 2023
@mhellmeier
Copy link
Member

Reopen as the inactivity might be related to the Christmas holiday season.

@mhellmeier mhellmeier reopened this Jan 3, 2024
@github-actions github-actions bot removed the stale label Jan 4, 2024
@arnoweiss
Copy link
Contributor

Please check for redundancy to this PR:
#953

Copy link
Contributor

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label Jan 12, 2024
Copy link
Contributor

This pull request was closed because it has been inactive for 7 days since being marked as stale.

@github-actions github-actions bot closed this Jan 19, 2024
@ndr-brt
Copy link
Contributor

ndr-brt commented Feb 23, 2024

I'm obviously incapable of properly using the dash-licence-tool. Here's what I tried:

  1. push, copy output from failed pipeline, paste, commit, push --> error
  2. copy from main, commit, push --> error
  3. run ./gradlew allDependencies | grep -Poh "(?<=\s)[\w.-]+:[\w.-]+:[^:\s]+" | sort | uniq | java -jar ./build/libs/dash.jar - -summary DEPENDENCIES after having downloaded the jar to ./build/libs, commit, push --> error

Any ideas or a quick fix?

yes, we noticed that on different machines (also with the same OS!) it gives different sorting.
just open the failed check, in the console output there will be the expected dependencies file printed, just copy and paste it

{
"odrl:action": "use",
"odrl:constraint": {
"@type": "LogicalConstraint",
Copy link

Choose a reason for hiding this comment

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

we should remove the not required LogicalConstraint in Usage Policies.

Copy link
Contributor

Choose a reason for hiding this comment

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

I said it before and I'll say it again: The implementation can handle logical constraints. If they are discouraged, this should be laid out in normative documents, not in descriptive ones like usage docs.

Copy link

@matgnt matgnt Feb 23, 2024

Choose a reason for hiding this comment

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

Then they must be able to handle both options. Example here:
eclipse-tractusx/tractusx-profiles#12

Both are ODRL compliant and have the same meaning. This is the thing that comes with ODRL ;-)

Copy link
Contributor

@jimmarino jimmarino Feb 23, 2024

Choose a reason for hiding this comment

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

we should remove the not required LogicalConstraint in Usage Policies.

To expand on what @arnoweiss said, we will absolutely not eliminate LogicalConstraint support from the Tractus-X EDC implementation. If a use case or dataspace member has a need for logical constraints, TX-EDC will be able to support them.

That is distinct from the question of interoperability. A dataspace such as Catena-X may decide the minimum bar for interoperability is that all participants must support an atomic constraint or a logical and constraint. In that case, I'll point out two things:

  1. The must-support statement does not restrict systems from implementing more than that. Most interoperability efforts I am familiar with take this approach.
  2. Removing LogicalConstraint support would remove support for and constraints and any multiplicity constraint. This would mean that policies could only be authored with atomic constraints, which would be a severe limitation.

This is also distinct from supporting the multiple serialization formats. For example removing the need for specifying @type, which would default to an and constraint.

Copy link

Choose a reason for hiding this comment

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

Yes, I agree, we should not "remove" it from EDC. that is clear. As long as EDC does also support ODRL allowed way of describing this with:

"constraint": [
             {
              "leftOperand": "cx-policy:FrameworkAgreement",
              "operator": "eq",
              "rightOperand": "traceability:v1.0"
            },
            ...
        ]

all is good and I only would like to see this in the documentation, that users are aware they need to be able to handle both. Otherwise, we would need to restrict how ODRL can be used.
@jimmarino Do we have any issue with EDC with the given example? When I tried, I couldn't' detect one with 0.5.3

Copy link
Contributor

Choose a reason for hiding this comment

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

Please verify this against the latest upstream EDC implementation and if it is not supported, file an enhancement request. The issue will then need to be triaged, accepted (or rejected), and prioritized by the committers.

Copy link

Choose a reason for hiding this comment

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

Ok I'm giving up on this. Taking this as tx-edc setting the rules here. It must be LogicalConstraint->and , independent from what may be allowed with ODRL.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not what I said. As I indicated, TX-EDC will continue to support multiple logical constraint types, including specifying them in ODRL serialized as Json-Ld. It is also reasonable for upstream EDC to support not specifying @type and interpreting that as a logical and constraint. However, if that is not supported, it should be submitted as a feature request to the EDC project.

This is how Eclipse open-source projects work. It's not about "tx-edc setting the rules." It's about providing proper governance overseen by the committers.

Copy link

Choose a reason for hiding this comment

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

This is how Eclipse open-source projects work. It's not about "tx-edc setting the rules." It's about providing proper governance overseen by the committers.

yes, correct. not the best wording I used. Thanks for the clarification @jimmarino

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.

The dash license tool pipeline is still failing. Please ensure that the DEPENDENCIES file is up to date.

@paullatzelsperger
Copy link
Contributor

paullatzelsperger commented Feb 27, 2024

@BenediktSR you have several open reviews. Could you re-request a review from the reviewers?
@mhellmeier thanks for pointing that out, but no need to request changes IMO. That is usually used when there is significant problem in the code.

@BenediktSR
Copy link
Contributor Author

As Arno has already said, there are always ongoing discussions on this topic. Arno is looking into this. He has agreed to revise the passages. Thanks a lot!!!

Copy link
Contributor

github-actions bot commented Mar 6, 2024

This pull request is stale because it has been open for 7 days with no activity.

@github-actions github-actions bot added the stale label Mar 6, 2024
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@arnoweiss arnoweiss requested review from mhellmeier and ndr-brt March 12, 2024 08:34
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 the mentioned issues and open points!

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.

(Approval related to my mentioned point of the outdated DEPENDENCIES file which has been solved)

@github-actions github-actions bot removed the stale label Mar 13, 2024
@ndr-brt
Copy link
Contributor

ndr-brt commented Mar 14, 2024

I re-requested review to the ones that commented but not approved, my proposal would be to merge this by tomorrow EOB if no one is against it, 4 months are an huge amount of time for a PR to stay open, further fixes can be done after then.

@ndr-brt
Copy link
Contributor

ndr-brt commented Mar 15, 2024

there we go

@ndr-brt ndr-brt merged commit cbcabb1 into eclipse-tractusx:main Mar 15, 2024
25 checks passed
@arnoweiss
Copy link
Contributor

thanks - I agree that PRs should never stay open for this long. it fell victim to uncoordinated improvement efforts for the docs. glad it's merged now. we'll do better in the future.

@BenediktSR
Copy link
Contributor Author

Tanks a lot guys, especially to Arno, who took the lead at the end. Yes, I think it was another learning experience for us!

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
Status: Merged
Development

Successfully merging this pull request may close these issues.

docs: new place for policy description manual