-
Notifications
You must be signed in to change notification settings - Fork 26
Add artifactId to service events #75
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
Conversation
Add the artifactId to some of the service events: - published - upgraded - rolledback The model is that one service is associated to one artifact. As already documented in the primer.md, this is a simplified model and it may change in future versions. The format of the artifactId in the examples is consistent with the rest of the spec for now. All artifactIds will align to the purl format in an upcoming pull request. Fixes: cdevents#58 Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
| | id | `String` | Uniquely identifies the subject within the source. | `service/myapp`, `daemonset/myapp` | | ||
| | source | `URI-Reference` | [source](../spec.md#source) from the context | `staging/tekton`, `tekton-dev-123`| | ||
| | environmentId | `String` | Id of the environment where the service runs | `dev`, `staging`, `production`, `ci-123`| | ||
| | artifactId | `String` | Identifier of the artifact deployed with this service | `0b31b1c02ff458ad9b7b81cbdf8f028bd54699fa151f221d1e8de6817db93427`, `927aa808433d17e315a258b98e2f1a55f8258e0cb782ccb76280646d0dbe17b5`, `six-1.14.0-py2.py3-none-any.whl` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the format of artifact IDs in an upcoming PR to address #38 (once this one is merged)
|
The PR comment should say "deployed" instead of "published", right? |
Nice catch, thank you, I will update the commit message |
|
Why isn't the artifactId a mandatory contents parameter? What use is it to send a service deployed event if it doesn't reference what is deployed? |
I was being conservative in adding new mandatory fields, but I'd be happy to switch that to mandatory |
|
I'd say it should be mandatory in 0.1. We could still change that in 0.2 or later (but not after 1.0 unless stepping the major version). I think we might have other major changes coming down the line before we reach 1.0 so it shouldn't be a huge drawback if we change our minds on this one later. |
Update the jsonschema to mark the environmentId as mandatory too. Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
Updated, it's mandatory now |
schemas/servicedeployed.json
Outdated
| ] | ||
| }, | ||
| "artifactId": { | ||
| "type": "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this string should have a minimum length specified, since it's a mandatory field, or?
| "type": "string" | ||
| }, | ||
| "url": { | ||
| "type": "string" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code change has nothing to with this PR, so it should probably have been contributed in another PR, but I'm fine with it anyway.
Signed-off-by: Andrea Frittoli <andrea.frittoli@gmail.com>
Changes
Add the artifactId to some of the service events:
The model is that one service is associated to one artifact. As already documented in the primer.md, this is a simplified model and it may change in future versions.
The format of the artifactId in the examples is consistent with the rest of the spec for now. All artifactIds will align to the purl format in an upcoming pull request.
Fixes: #58
Signed-off-by: Andrea Frittoli andrea.frittoli@gmail.com
Submitter Checklist
As the author of this PR, please check off the items in this checklist: