Skip to content

Conversation

dtfranz
Copy link

@dtfranz dtfranz commented Aug 28, 2025

Captures conditions and reasons used by ClusterExtensionRevision into consts.

Description

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@@ -39,7 +39,6 @@ const (
TypeServing = "Serving"

// Serving Reasons
ReasonAvailable = "Available"
Copy link
Author

Choose a reason for hiding this comment

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

Moved to common_types.go

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should actually prefix the types/seasons with the API kind they go with so that it is more clear, and less likely that we'll accidentally use a type that was intended for another API.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking we could go that way too, but I ended up going the route of sharing the ones that are common. Not a strong preference at all though and I'm happy to do it the other way, too. Your suggestion also makes more sense if we ever changed the value of one and didn't want it to propagate to every API that uses it.

Copy link
Author

Choose a reason for hiding this comment

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

I've updated this particular field so it's not generic, but to more specifically address your comment would you like something like this?

	// Condition Types
	ClusterExtensionRevisionTypeAvailable = "Available"
	ClusterExtensionRevisionTypeSucceeded = "Succeeded"

	// Condition Reasons
	ClusterExtensionRevisionReasonObjectAvailable           = "Available"
	ClusterExtensionRevisionReasonReconcileFailure          = "ReconcileFailure"

Copy link
Collaborator

Choose a reason for hiding this comment

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

My suggestion is:

<RootKind>Type<Type> = "<Type>"
<RootKind>Reason<Reason> = "<Reason>"

I think we even have a unit test that enforces that the const name suffix matches its string value (and it may have to be adjusted to account for the Kind prefix)

Copy link
Collaborator

Choose a reason for hiding this comment

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

My main motivation is less about how changing one might accidentally mean changing others, and more about making it less likely that we accidentally use a type or reason in one API when it was never meant to be used for that API in the first place.

So if we have TypeAvailable and it is meant for ClusterExtensionRevision only, it would be pretty easy to imagine another PR that comes in later that changes the ClusterExtension status to use TypeAvailable and we miss that in a review.

It would be easier to catch in a review (and less likely to be committed in the first place) if it was called ClusterExtensionRevisionTypeAvailable.

And if we ever needed to actually have "Available" as a type for ClusterExtension, that would be fine: we'd add ClusterExtensionTypeAvailable = "Available".

Copy link
Collaborator

@joelanford joelanford Sep 4, 2025

Choose a reason for hiding this comment

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

Super quick glance and I only see test code for type/reason registration in the ClusterExtension unit tests? If that's true, perhaps we should capture another TODO Jira issue to expand that unit test to cover types and reasons for all of our APIs.

@dtfranz dtfranz changed the title Const Cleanup 🌱 Const Cleanup Aug 28, 2025
@codecov-commenter
Copy link

codecov-commenter commented Aug 29, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 75.75758% with 8 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (poc-boxcutter@01cf0d3). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...controllers/clusterextensionrevision_controller.go 72.41% 8 Missing ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files
@@               Coverage Diff                @@
##             poc-boxcutter      #10   +/-   ##
================================================
  Coverage                 ?   71.87%           
================================================
  Files                    ?       85           
  Lines                    ?     8153           
  Branches                 ?        0           
================================================
  Hits                     ?     5860           
  Misses                   ?     1899           
  Partials                 ?      394           
Flag Coverage Δ
e2e 39.95% <0.00%> (?)
experimental-e2e 44.69% <57.57%> (?)
unit 56.47% <63.63%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment on lines 32 to 41
// Condition Reasons
ReasonObjectAvailable = "Available"
ReasonReconcileFailure = "ReconcileFailure"
ReasonRevisionValidationFailure = "RevisionValidationFailure"
ReasonPhaseValidationError = "PhaseValidationError"
ReasonObjectCollisions = "ObjectCollisions"
ReasonRolloutSuccess = "RolloutSuccess"
ReasonProbeFailure = "ProbeFailure"
ReasonIncomplete = "Incomplete"
ReasonProgressing = "Progressing"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this PR faithfully creates constants for everything that we're actually using, and it is almost the point of this PR to centralize this and make it plainly obvious what our true surface is. So this is what I expect of this PR! :)

So now that we see this, here are meta questions meant for all of us:

  1. do we need this many reasons?
  2. are these the "right" reasons? (for example, I added "ReconcileFailure" on a whim just to hack the status plumbing together)

TypeSucceeded = "Succeeded"

// Condition Reasons
ReasonObjectAvailable = "Available"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should stick with the pattern Reason<Reason> = "<Reason>"

Suggested change
ReasonObjectAvailable = "Available"
ReasonAvailable = "Available"

Copy link
Author

Choose a reason for hiding this comment

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

ReasonAvailable is already set in clustercatalog_types.go, which was why initially I moved that one to the common_types.go to share it with CER. So we either need to combine them like I did before or have another distinct name, though it doesn't have to be ReasonObjectAvailable, of course 😃

Copy link
Author

Choose a reason for hiding this comment

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

...although following your recommendation here, this will resolve itself if I name it ClusterExtensionRevisionReasonAvailable.

Captures conditions and reasons used by ClusterExtensionRevision into consts.

Signed-off-by: Daniel Franz <dfranz@redhat.com>
@thetechnick thetechnick merged commit 24e345a into thetechnick:poc-boxcutter Sep 4, 2025
14 of 15 checks passed
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.

4 participants