-
Notifications
You must be signed in to change notification settings - Fork 50
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
Treat empty runtime namespaces as All Namespaces mode #658
Conversation
/cc @metacosm |
Looks reasonable, was thinking about doing something similar indeed. Would you mind changing the commit message so that it fits the conventional format, though, please? |
3279685
to
7878952
Compare
Like this? Happy to. Hope you're enjoying your vacation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if we can't support this in a simpler way, a test case would help figure this out… Otherwise the general idea is the right one, I think.
core/runtime/src/main/java/io/quarkiverse/operatorsdk/runtime/ConfigurationServiceRecorder.java
Outdated
Show resolved
Hide resolved
// We use a default here so that we are able to detect if the configuration value is set to "". Setting the value to "" will | ||
// reset the configuration and result in an empty Optional, but not setting the value at all will result in the default being | ||
// applied. | ||
@ConfigItem(defaultValue = QOSDK_USE_BUILDTIME_NAMESPACES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it might not just be enough to set the default value to io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_ALL_NAMESPACES
here. Ideally, we'd need a test to cover this, actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we set the default to WATCH_ALL_NAMESPACES we couldn't tell if there was no specific runtime setting, which means we would always override the build time property.
Maybe the build time property doesn't matter because it's only really intended to affect the manifests and not the runtime behaviour... but at the moment the code will honour the build time property if the runtime property is not set at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the build time property doesn't matter because it's only really intended to affect the manifests and not the runtime behaviour... but at the moment the code will honour the build time property if the runtime property is not set at all.
I would say that this is the intended behavior but I'm open to change my mind about this depending on what people think. The previous version used the same property for both build and runtimes, which was conducive to confusing which version was used ultimately. With the new generate-with-watched-namespaces
property, it is now more explicit that the build time value is used mostly for generation purposes. Its value can indeed be propagated at runtime and it's indeed how things are currently implemented, just not sure if it actually make sense since it would make it more difficult to know exactly which configuration is used at runtime. Completely separating both might make more sense with that respect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my thinking with a property that was explicitly namespaced with manifests
in the name to separate it from properties that affect the built image.
I feel like the manifests are really runtime configuration and tbh, I think using quarkus.operator-sdk.namespaces at build time should affect the manifests and not the image, and that would make conceptual sense. It breaks the duplicated properties rule though.
If the runtime configuration specifies an empty string as the namespaces to watch, we should treat that as all namespaces mode, mainly because that's how OLM signifies that an operator should watch all namespaces. An empty string is treated the same as unset by the configuration parser, so we need to set a default on the field so we can explicitly check for it being unset, to determine if the empty string was used. Fixes quarkiverse#656 Signed-off-by: James Hewitt <[email protected]>
7878952
to
310ee91
Compare
I had a look at how to test this - at the moment the integration tests run a single operator with all the test controllers injected. If I were to set the global operator namespace property for those tests, it would break all the existing tests. What are your thoughts on how to architect a test here? A second set of integration tests? A second profile? Something else? |
I'm thinking about specific tests using a specific test profile. |
Thank you. I will add tests in a separate PR. |
Thanks Chris :) I did have a look at it, but think I'd need more time for the profiles thing! |
This makes the empty string in the namespaces configuration watch all namespaces, as per the documented behaviour: https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/core/runtime/src/main/java/io/quarkiverse/operatorsdk/runtime/RunTimeOperatorConfiguration.java#L56
Fixes #656