Skip to content

Conversation

matheuscscp
Copy link
Member

@matheuscscp matheuscscp commented Oct 10, 2025

WatchExternalArtifacts: true,
WatchConfigsPredicate: predicate.Not(predicate.Funcs{}),
WatchExternalArtifacts: true,
CancelHealthCheckOnNewRevision: true,
Copy link
Member Author

Choose a reason for hiding this comment

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

How to have tests for both true and false here?

Copy link
Member

Choose a reason for hiding this comment

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

See the lines you just deleted from the test:

	reconciler.CancelHealthCheckOnNewRevision = true
	t.Cleanup(func() { reconciler.CancelHealthCheckOnNewRevision = false })

Copy link
Member Author

Choose a reason for hiding this comment

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

See the boolean moving from the reconciler to the SetupWithManager options

Copy link
Member

Choose a reason for hiding this comment

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

Ah right, the test framework we have in place does not cope with this, you can not rebuild the controller once it has started.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe a separate package like we did before?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or maybe test only true since ideally we would not have this feature gate. If this works well we plan to make it opt-out later, and possibly even ignore it with a warning after even more time.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah enabling it in tests should be Ok. The e2e test suite has the default gate state, so we cover both cases.

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.

2 participants