events: replace string literals with variables

[clobrano]

Add variables for event reasons and messages to allow reuse of the same
messages and help tests.

Signed-off-by: Carlo Lobrano [email protected]

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: clobrano

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [clobrano]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[clobrano]

Following this discussion, this change can be useful when an operator has un-common events that fall into the same category of other "common" events already available as API (e.g. RemediationCannotStart)

[slintes]

this is very inconsistent :/

func name: GetTargetNodeFailed
reason: RemediationCannotStart
message: var name RemediationCannotStart, but value about the target node

IMHO most useful would be:
func name: RemediationCannotStart
reason: RemediationCannotStart
msg: passed in by parameter, there probably can be multiple reasons for a remediation to not start

Changing this would be an API change though, which would, when done correctly, need a major version bump...

So my suggestion is:

• rename the msg var to e.g. RemediationCannotStartTargetNodeFailedEventMessage , to make its meaning a bit clearer
• add a function as suggested above, and use it inside GetTargetNodeFailed?

WDYT?

Also, while I agree that the reasons should be consistent and might be worth adding to the API, I don't see that for the messages. What's the motivation for making them public?

[clobrano]

What's the motivation for making them public?

Mostly testing, but I'm still torn about leaving this information available only for testing and I won't mind reverting this particular bit 🤷

this is very inconsistent :/

I agree. I dunno why I choose a function name so different than the others :/

rename the msg var to e.g. RemediationCannotStartTargetNodeFailedEventMessage , to make its meaning a bit clearer
etc.

sounds like a good idea :)

[slintes]

This is better now, lgtm
Going to leave it open for others to review who use this more than I do :)

dismissing a review for getting rid of requesting changes, does it work? 🤷

[razo7]

/lgtm
Just one small NIT on function declaration of RemediationStarted, RemediationStoppedByNHC, RemediationFinished, and GetTargetNodeFailed. Since they no longer have a fixed message inside and they have a private variable of the message, thus their function declaration should fit with more vague information instead of specific (e.g., and message "Remediation finished" -> and a general message, or just remove the comment on the message).

[clobrano]

Since they no longer have a fixed message inside

those functions still have a fixed message. Only RemediationCannotStart has a custom message

[razo7]

Since they no longer have a fixed message inside

those functions still have a fixed message. Only RemediationCannotStart has a custom message

Correct, and I was referring to the other functions :)
I am trying to say that before we had an inline string with the exact message inside the function declaration (e.g., "Remediation finished"), so it was correct and straightforward, and now we have a const.
If in the future this const will be changed then we would have to remember updating the respected function declaration. I think we can reduce one less thing to remember in case future us would like to change a simple const without confusing/unaligning with the function declaration.
I hope my motivation is clearer now.

[clobrano]

I hope my motivation is clearer now

Yes, thank you, now I understand it better and it makes sense

[razo7]

/lgtm