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

Add change.created description field #155

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

xibz
Copy link
Contributor

@xibz xibz commented Aug 14, 2023

Changes

This commit adds a new optional field to the dev.cdevents.change.created
event. This field, description, should be used to describe the change,
e.g. a GitHub PR.

An example of the description field could be something like

"This PR addresses a new bug that was introduced in some PR"

resolves #151

Signed-off-by: benjamin-j-powell benjamin_j_powell@apple.com

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

@xibz
Copy link
Contributor Author

xibz commented Aug 14, 2023

Has spec version and event versions been updated according to the versioning policy

@afrittoli @e-backmark-ericsson One thing that wasn't clear to me was the versioning. Can you explain the process? Since the link looks to 404. We should fix that as well and document the process.

@xibz xibz force-pushed the change-created-description branch from ce4784d to 9e82af9 Compare August 14, 2023 07:18
@e-backmark-ericsson
Copy link
Contributor

@xibz , looks like the title of this PR has been copied from another PR. Could you please correct that?

@xibz xibz changed the title introduce links to CDEvents Add change.created description field Aug 14, 2023
@@ -156,6 +156,7 @@ A source code change was created and submitted to a repository specific branch.
|-------|------|-------------|----------|----------------------------|
| id | `String` | See [id](spec.md#id-subject)| `1234`, `featureBranch123` | ✅ |
| source | `URI-Reference` | See [source](spec.md#source-subject) | `my-git.example/an-org/a-repo`| |
| description | `String` | Description associated with the change, e.g. A pull request's description | `This PR addresses a fix for some feature`| |
Copy link
Contributor

Choose a reason for hiding this comment

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

This description field should also be mentioned in the change table (e.g. on line 61 above).
Also, we want to keep the id, source and type fields together, so please move this new field one step down in the table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I think I did that correctly. It's a little odd that it seems to be a complete duplicate of the line I added.

Copy link
Contributor

@e-backmark-ericsson e-backmark-ericsson Aug 14, 2023

Choose a reason for hiding this comment

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

It's a little odd that it seems to be a complete duplicate of the line I added.

I do agree on that. We should maybe refactor that in the future because it is a bit hard to grasp, but that is what it looks like today at least.

@e-backmark-ericsson
Copy link
Contributor

Has spec version and event versions been updated according to the versioning policy

@afrittoli @e-backmark-ericsson One thing that wasn't clear to me was the versioning. Can you explain the process? Since the link looks to 404. We should fix that as well and document the process.

The versioning of cdevents is documented here: https://cdevents.dev/docs/primer/#versioning
You should step the minor version of the change.created event from 0.1.2 to 0.2.0 and keep the spec version as 0.4.0-draft. It's correct that the $id field in the schema will not be possible to resolve until the new spec version is released (and then all files will be updated to 0.4.0 as part of the release as well)

@xibz xibz force-pushed the change-created-description branch from d3e4178 to 2a2296f Compare August 14, 2023 15:27
Copy link
Contributor

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

Hmm. I'm a bit confused myself over the structure of the json bodies. New subject specific fields should always be placed within the content object in the subject object. The top level properties in a subject should only be id, source, type and content for all event types.

@@ -58,6 +58,10 @@
],
"default": "change"
},
"description": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this down to be part of the content object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, what's the top level id, source, and type not doing in content?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the reasoning is that those fields are identifying the subject itself, while any other fields are considered parameters on the subject identity.

@xibz xibz force-pushed the change-created-description branch from b841dd4 to c3efcaa Compare August 14, 2023 16:44
@e-backmark-ericsson
Copy link
Contributor

The only thing missing on this PR is that the commit needs to be signed.
https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@xibz xibz force-pushed the change-created-description branch from c3efcaa to 2ba8074 Compare August 15, 2023 08:49
This commit adds a new optional field to the dev.cdevents.change.created
event.  This field, description, should be used to describe the change,
e.g. a GitHub PR.

An example of the description field could be something like

"This PR addresses a new bug that was introduced in some PR"

Signed-off-by: xibz <impactbchang@gmail.com>
@xibz xibz force-pushed the change-created-description branch from 2ba8074 to 061a46e Compare August 15, 2023 09:07
@xibz
Copy link
Contributor Author

xibz commented Aug 15, 2023

The only thing missing on this PR is that the commit needs to be signed.
docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@e-backmark-ericsson done!

@e-backmark-ericsson e-backmark-ericsson merged commit b6df4e2 into cdevents:main Aug 15, 2023
xibz added a commit to xibz/cdevents-spec that referenced this pull request Oct 13, 2023
This commit adds a new optional field to the dev.cdevents.change.created
event.  This field, description, should be used to describe the change,
e.g. a GitHub PR.

An example of the description field could be something like

"This PR addresses a new bug that was introduced in some PR"

Signed-off-by: xibz <impactbchang@gmail.com>
Co-authored-by: benjamin-j-powell <benjamin_j_powell@apple.com>
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.

created change event should contain a top level description
3 participants