Skip to content
This repository was archived by the owner on Dec 21, 2023. It is now read-only.

Conversation

johannes-b
Copy link
Member

@johannes-b johannes-b commented May 15, 2020

This PR proposes a change of the spec in two aspects:

  • remediation.yaml configuration
  • CloudEvents for remediation workflow

@johannes-b johannes-b changed the title Implementation of spec change as proposed in KEP 09 SPEC 31 - Implementation of spec change as proposed in KEP 09 May 15, 2020
cloudevents.md Outdated
"remediation": {
"status": "succeeded",
"result": {
"finishedActionIndex": 0,
Copy link
Member

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)?

Copy link
Member Author

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

Copy link
Member

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": {
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

@johannes-b johannes-b Jun 8, 2020

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.

Copy link
Member

@christian-kreuzberger-dtx christian-kreuzberger-dtx left a comment

Choose a reason for hiding this comment

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

lgtm

@christian-kreuzberger-dtx christian-kreuzberger-dtx merged commit 8dd8366 into master Jun 30, 2020
@christian-kreuzberger-dtx christian-kreuzberger-dtx deleted the proposal/remediation-action branch June 30, 2020 09:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants