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.

Sign up for GitHub

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

Jump to bottom

build: Add LifecycleScopedRelationship constraints #517

Merged
merged 2 commits into from
Nov 28, 2023
Merged

build: Add LifecycleScopedRelationship constraints #517

merged 2 commits into from
Nov 28, 2023

Conversation

nishakm
Copy link
Collaborator

@nishakm nishakm commented Oct 17, 2023

No description provided.

@nishakm
Copy link
Collaborator Author

nishakm commented Oct 17, 2023

Related PRs:

goneall
goneall approved these changes Oct 17, 2023
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

LGTM - Just need to wait for the spec parser to handle the OR's before merging.

@nishakm
Copy link
Collaborator Author

nishakm commented Oct 17, 2023

I've updated the PR so the restrictions are on the Classes/Build.md file. However, I may not have entered the restrictions according to what spec-parser wants. This is on purpose because the constraints are not agreed upon.
@zvr

kestewart
kestewart approved these changes Oct 21, 2023
View reviewed changes
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Need to have further discussions, but applying this so we can see it all in context, for the relationships.

@kestewart
Copy link
Contributor

@nishakm, @zvr - can you look into the parser error, and see if you can sort it so we can get this merged.

@zvr
Copy link
Member

zvr commented Oct 21, 2023

@kestewart the parser does not understand this new section ## External LifecycleScopedRelationship Constraints describing validation.

We have not even finalized how such constraints should be expressed -- #522 has to be fleshed out, discussed and resolved for this.
My personal view is that having class names inside the section title is not the way to go.

goneall
goneall requested changes Oct 21, 2023
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

A couple of suggested changes to be compatible with the proposed restrictions syntax.

Note that the proposal hasn't been finalized, so we may end up with another round of updates.

model/Build/Classes/Build.md Outdated Show resolved Hide resolved
model/Build/Classes/Build.md Outdated
## External LifecycleScopedRelationship Constraints

- relationshipType: inputOf OR buildTool OR configOf
- toType: /Build/Build
Copy link
Member

Choose a reason for hiding this comment

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

In the discussion on the relationship type restrictions, we decided to only have a fromType sub-bullet since the toType is typically restricted by the relationshipType across all profiles. I'm thinking the from type would be /Core/Artifact.

@goneall goneall added this to the 3.0-rc2 milestone Nov 7, 2023
@goneall
Copy link
Member

goneall commented Nov 7, 2023

@zvr - Need your input on as well

@goneall
Copy link
Member

goneall commented Nov 7, 2023

Ping @nishakm

@zvr
Copy link
Member

zvr commented Nov 7, 2023

@nishakm such conformance conditions are to be expressed in plain language in the top-level namespace file.

See #524 for an example.

zvr
zvr requested changes Nov 7, 2023
View reviewed changes
model/Build/Classes/Build.md Outdated Show resolved Hide resolved
kestewart
kestewart requested changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Need to get the relationships consistent across the profiles. See #556

@nishakm
Copy link
Collaborator Author

nishakm commented Nov 21, 2023

@kestewart once #556 is merged, I can update this PR accordingly.

@nishakm
build: Add LifecycleScopedRelationship constraints
5209405 
Signed-off-by: nisha <nisha@ctlfsh.tech>
@nishakm
Copy link
Collaborator Author

nishakm commented Nov 21, 2023

@kestewart Done

goneall
goneall approved these changes Nov 28, 2023
View reviewed changes
Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

LGTM

kestewart
kestewart approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@kestewart kestewart left a comment

Choose a reason for hiding this comment

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

Thanks Nisha, this matches what we discussed last week.

zvr
zvr requested changes Nov 28, 2023
View reviewed changes
model/Build/Build.md Outdated Show resolved Hide resolved
@goneall @zvr
Update model/Build/Build.md per review comments
e9b032b 
Co-authored-by: Alexios Zavras (zvr) <zvr+git@zvr.gr>
@goneall goneall merged commit a16676c into spdx:main Nov 28, 2023
1 check passed
@goneall goneall mentioned this pull request Mar 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvr zvr zvr approved these changes

@goneall goneall goneall approved these changes

@kestewart kestewart kestewart approved these changes

Assignees
No one assigned
Labels
Profile:Build
Projects
None yet
Milestone
3.0-rc2
Development

Successfully merging this pull request may close these issues.
4 participants
@nishakm @kestewart @zvr @goneall