-
Notifications
You must be signed in to change notification settings - Fork 10
SPEC 31 - Implementation of spec change as proposed in KEP 09 #31
SPEC 31 - Implementation of spec change as proposed in KEP 09 #31
Conversation
cloudevents.md
Outdated
"remediation": { | ||
"status": "succeeded", | ||
"result": { | ||
"finishedActionIndex": 0, |
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.
these fields should be called actionIndex
and actionName
, right (see specification above)?
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.
Correct, there is a mistake in the example. The fields should not reflect a state of the event. The event state is indicated by the event type (.triggered/.started/.status.changed./.finished).
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.
Thank you! I corrected the spec.
"status": { // Enum: succeeded, errored, unknown | ||
"type": "string" | ||
}, | ||
"result": { |
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.
According to Issue #1849, it should also possible to include arbitrary strings within the result (e.g. "remediation file not configured"). Should we add an additional field (e.g. message
) here, or should we remove the enum restriction?
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 would propose to introduce an additional message
field. Otherwise, the event receiver would need to correctly interpret the result message. This can lead to misinterpretation.
The same is true for the status
field.
Should we add: statusMessage
, resultMessage
(which are optional)?
@agrimmer What is your opinion on that?
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 would prefer introducing a new field "message". This would allow us to use the "result" field for the evaluation result.
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.
My proposal of having a dedicated statusMessage
and resultMessage
is over-engineered since there cannot be a result when the status is errored
or unknown
. As a result, just one message
field is sufficient.
a7ebfe3
to
dfd27d1
Compare
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.
lgtm
This PR proposes a change of the spec in two aspects: