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

Introduced purls instead of GAVs #195

Merged
merged 6 commits into from
Sep 11, 2018

Conversation

d-stahl-ericsson
Copy link
Contributor

@d-stahl-ericsson d-stahl-ericsson commented Aug 7, 2018

Applicable Issues

#182

Description of the Change

Replaced GAVs for meta.source.serializer in all events, and for data.identity (instead of data.gav) and data.implements and data.dependsOn in EiffelArtifactCreatedEvent.

Updated schema and examples accordingly. In doing so, realized that usage-examples/delivery-interface has not been updated since EiffelIssueDefinedEvent was introduced, and so fixed this.

Gliffy is rather old and poorly maintained. Converted the diagram source file from .gliffy to a draw.io .xml file.

Alternate Designs

purl is not the only game in town. There's also e.g. SWID, but practical applications do not abound. Nevertheless, if other standards arise I believe we are well equipped to integrate them in a flexible manner: by using "identity" as a property name we stay agnostic. The string, currently prefixed "pkg:" as per purl specification could then be used for other identification schemes in the future (e.g. SWID tags) by similarly prefixing a string format.

Benefits

This gets rid of rather awkward Maven legacy that has been around for way too long. Maven was the best standard we had to fall back on in the very early days, but now this tight coupling to Maven is a weakness that needs to be fixed.

Possible Drawbacks

purl might not turn out to be the future universal standard, but it's pragmatic and usable for a wide variety of packaging types already. If a clear "winner" materializes down the road, see above on flexibility.

Sign-off

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

Signed-off-by: Daniel Ståhl daniel.stahl@ericsson.com


### data.fileInformation
__Type:__ Object[]
__Required:__ No
__Description:__ A list of the artifact file contents, following the standard established by [Apache Maven](https://maven.apache.org/pom.html).
__Description:__ A list of the artifact file contents. This information is optional, and when included, MAY include a complete or incomplete list of contents. In other words, it may be used to highlight only particular files of interest, such as launcher binaries or other entry-points.
Copy link
Member

Choose a reason for hiding this comment

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

If I know my grammar correctly it should be '...optional and, when included, MAY include...'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

__Type:__ String
__Required:__ Yes
__Description:__ The classifier of the file within the artifact. If none, an empty string shall be used.
__Description:__ The name (including relative path from the root of the artifact) on syntax appropriate for the artifact packaging type.
Copy link
Member

Choose a reason for hiding this comment

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

typo: '... or syntax ...', right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. The name should be on appropriate syntax. It's not that the property contains names or syntax.

Choose a reason for hiding this comment

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

True

__Type:__ String
__Required:__ Yes
__Description:__ The groupId of the dependency.
__Description:__ An array of [purl identified](https://github.com/package-url/purl-spec) entities this artifact implements. The typical use case of this is to identify interfaces implemented by this artifact. Note that [version range notation](https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402) may be used for the version component of the package identity.
Copy link
Member

Choose a reason for hiding this comment

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

There seems to be no version range syntax defined in purl but what it states is that the version field should be percent-encoded. I guess we should then complement the Note in this section with that the given version range needs to be percent-encoded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, I attempted to clarify this in the most recent commit.

__Description:__ The version of the dependency. Note that [version range notation](https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402) is supported.
__Format:__ [purl specification](https://github.com/package-url/purl-spec)
__Required:__ No
__Description:__ An array of [purl identified](https://github.com/package-url/purl-spec) entities this artifact depends on. Note that [version range notation](https://docs.oracle.com/middleware/1212/core/MAVEN/maven_version.htm#MAVEN402) may be used for the version component of the package identity.
Copy link
Member

Choose a reason for hiding this comment

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

See comment on data.implements above

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 above.

@e-backmark-ericsson
Copy link
Member

I embrace this change (with the comments sorted out). Anyway, I think someone with more experience in managing Eiffel events in Maven heavy applications should have a say about potential drawbacks. @emichaf ?

And, about moving from Gliffy to draw.io. I guess it's a good switch, but I'd prefer that it was managed in a separate PR where all diagrams where converted at once.

@d-stahl-ericsson
Copy link
Contributor Author

And, about moving from Gliffy to draw.io. I guess it's a good switch, but I'd prefer that it was managed in a separate PR where all diagrams where converted at once.

You're right, of course. I was simply being lazy. Once #199 is merged I'll rebase to that.

@e-backmark-ericsson
Copy link
Member

I'm fine with the latest commit.

Replaced GAVs for meta.source.serializer in all events,
and for data.identity (instead of data.gav) and data.implements
and data.dependsOn in EiffelArtifactCreatedEvent.

Updated schema and examples accordingly. In doing so, realized
that usage-examples/delivery-interface has not been updated
since EiffelIssueDefinedEvent was introduced, and so fixed this.

Gliffy is rather old and poorly maintained. Converted the
diagram source file from .gliffy to a draw.io .xml file.
@d-stahl-ericsson
Copy link
Contributor Author

Rebased and updated the delivery-interface.svg.

Copy link
Member

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

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

Main conversation item should to be updated? It refers to draw.io file

These files still mention GAV:

  • eiffel-syntax-and-usage/compositions-and-validity-checking.md
  • usage-examples/confidence-level-joining.md

Version ranges need to be percent-encoded. The documentation is fine, but some examples need to be updated:

  • examples/events/ArtifactCreatedEvent/backend.json:implements
  • examples/events/ArtifactCreatedEvent/dependent.json:dependsOn: The version range should be percent-encoded
  • svg files as well, or should the ranges mentioned there be considered schematic? (composition-dependency-check-example.svg and composition-backend-implementation-example.svg)

This PR creates non-backwards compatible changes to all events. See issue #200. I'd like to see that that issue is closed before this PR is merged.

compositions-and-validity-checking.md and confidence-level-joining.md
mentioned GAVs. These references have been replaced. Also a typo was
fixed in compositions-and-validity-checking.md.
Made version ranges percent encoded in event examples. Added a note
in compositions-and-validity-checking.md that version ranges are
not percent encoded in the documentation, for readability reasons.
@d-stahl-ericsson
Copy link
Contributor Author

Main conversation item should to be updated? It refers to draw.io file

Nah, that was true at the time. The eventual commit message will be different.

These files still mention GAV

Good catch! I grepped for GAV and found no further instances in the repo, apart from event version history.

Version ranges need to be percent-encoded. The documentation is fine, but some examples need to be updated

I fixed the examples and grepped for other instances of "dependsOn" and "implements" in the repo, but found none (except schemas). In the documentation I added a note that version ranges are not percent encoded for readability reasons.

This PR creates non-backwards compatible changes to all events. See issue #200. I'd like to see that that issue is closed before this PR is merged.

Fair enough.

Copy link
Member

@e-backmark-ericsson e-backmark-ericsson left a comment

Choose a reason for hiding this comment

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

The changes look good to me.
Just waiting for #200 to be sorted out before merge.

@d-stahl-ericsson
Copy link
Contributor Author

#189 and #200 have been addressed. Are we good to go?

@e-backmark-ericsson
Copy link
Member

I'm glad you asked, and not merged right away :)
I will check with some people implementing services for the protocol to see if they would accept this change, since it affects the possibility to patch earlier versions/editions easily, and this specific change might not be rated very high in priority amongst them.

@rogpers-ericsson
Copy link

I approve the general move away from a Maven-centric view of artifacts.

If possible to get in before GA then this would be a replacement of the old method. If not, then the old solution should be deprecated (not removed) for some time, with the purl-approach being the preferred solution.

@d-stahl-ericsson d-stahl-ericsson merged commit dc5ec6f into eiffel-community:master Sep 11, 2018
@magnusbaeck magnusbaeck added protocol All protocol changes protocol-incompat Protocol changes that aren't backwards compatible labels Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
protocol All protocol changes protocol-incompat Protocol changes that aren't backwards compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants