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 Processing Condition labels #6

Conversation

clobrano
Copy link
Contributor

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

@clobrano clobrano requested a review from mshitrit July 12, 2023 09:57
@mshitrit
Copy link
Member

/lgtm
/hold
Nit: maybe also add nhc TimeOut Annotation ("remediation.medik8s.io/nhc-timed-out")

@clobrano clobrano force-pushed the add-processing-and-succeeding-condition-type-labels branch from ce00bf2 to e93c61a Compare July 12, 2023 15:29
// SucceededConditionType is the condition type used to signal whether the remediation was successful or not
SucceededConditionType = "Succeeded"
// NhcTimeOutAnnotation is the annotation set by NHC to signal the operator that it surpassed its timeout and shall stop its remediation
NhcTimeOutAnnotation = "remediation.medik8s.io/nhc-timed-out"
Copy link
Member

Choose a reason for hiding this comment

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

  1. think it would be better to put in another pkg (i.e annotations)
  2. Assuming we do so get rid of the "Annotation" suffix in the var name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I've been too fast, you're right

@clobrano clobrano force-pushed the add-processing-and-succeeding-condition-type-labels branch from e93c61a to 977b2e8 Compare July 13, 2023 09:30
@mshitrit
Copy link
Member

/lgtm

@clobrano clobrano marked this pull request as ready for review July 17, 2023 06:27
@clobrano clobrano merged commit e064487 into medik8s:main Jul 17, 2023
1 check passed
@clobrano clobrano deleted the add-processing-and-succeeding-condition-type-labels branch July 17, 2023 06:33
@slintes
Copy link
Member

slintes commented Jul 18, 2023

missed this. Why are the condition types in labels.go? 🤔 @clobrano

@clobrano
Copy link
Contributor Author

I think I considered labels.go as a place for strings, but after this simple question it's clearly the wrong place 🤦

What could be a good name for the right package?
status_conditions.go maybe?

@slintes
Copy link
Member

slintes commented Jul 18, 2023

just conditions.go would be fine IMHO, they are always in the status

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.

3 participants