feat: update github adapter spec #1306
Conversation
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
Signed-off-by: Remi Cattiau <[email protected]>
| | `datacontentencoding` | Omit | | ||
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | Omit | |
There was a problem hiding this comment.
I wonder if this should be repository.id ?
There was a problem hiding this comment.
The source is already repository.url
There was a problem hiding this comment.
ok, that's probably good enough
cloudevents/adapters/github.md
Outdated
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "package.id" value | | ||
| | `time` | "package.(created\|updated)\_at" value | |
There was a problem hiding this comment.
it's interesting that the doc says that "created_at" can be null - I wonder how that can be.
I wonder if we should say the value should be taken in this order: updated_at, created_at, current time
There was a problem hiding this comment.
Yeah GH event can be a bit 'weird' i like the updated_at|created_at|now() but a bit painful to write
| | `datacontentencoding` | Omit | | ||
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "repository.name" value | |
There was a problem hiding this comment.
The id is really not that useful to filter on
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "repository.name" value | | ||
| | `time` | "repository.updated_at" value | |
There was a problem hiding this comment.
or current time if not present?
There was a problem hiding this comment.
Should we add a comment on the top that claim for time attribute if null or invalid fallback to current time?
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "alert.id" value | | ||
| | `time` | "alert.updated_at" value or "alert.created_at" | |
There was a problem hiding this comment.
might want to add something about updated_at should be used unless null
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "alert.id" value | | ||
| | `time` | "alert.updated_at" value or "alert.created_at" | |
There was a problem hiding this comment.
same as above... updated_at if not null
| | `datacontentencoding` | Omit | | ||
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | Omit | |
There was a problem hiding this comment.
Same as first comment, we have the repository url in source, not sure we need the id in subject
| | `datacontentencoding` | Omit | | ||
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "workflow_job.name" value | |
There was a problem hiding this comment.
I wonder if workflow_job.id is better in the case of a rename?
There was a problem hiding this comment.
Most of GH ui only show the name, so my assumption is name would be more useful.
I do not know if the id change or not on rename
There was a problem hiding this comment.
what about workflow_job.run_url?
| | `datacontentencoding` | Omit | | ||
| | `datacontenttype` | `application/json` | | ||
| | `dataschema` | Omit | | ||
| | `subject` | "workflow.name" value | |
There was a problem hiding this comment.
I can't figure out of "name" need to be unique or not. I'm assuming 'id' must be.
From co-pilot:
the name field in a GitHub workflow does not need to be unique. The name is used for display purposes and helps you identify the workflow in the GitHub Actions tab
There was a problem hiding this comment.
actually, should this be the workflow.url ?
|
@loopingz any update on this? |
Signed-off-by: Remi Cattiau <[email protected]>
|
Conditionally approved on the 8/29 call - once the open comments are closed |
|
This Pull Request is stale because it has been open for 30 days with |
|
@loopingz did you want to reply to the open comments? |
Proposed Changes