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

fix: able to override controller level namespace when set in annotation #692

Closed
wants to merge 2 commits into from

Conversation

csviri
Copy link
Contributor

@csviri csviri commented Aug 21, 2023

When the namespace of a controller is set to a different value, for example:

@ControllerConfiguration(namespaces = WATCH_CURRENT_NAMESPACE, ... )

Then the quarkus.operator-sdk.namespaces configuration override is not working, thus setting the namespace in the annotation we should consider more like a default value.

@csviri csviri self-assigned this Aug 21, 2023
@metacosm
Copy link
Member

What use case is this suppose to fix, as this looks suspicious, the parameter that is now always false had been introduced for a reason.

@csviri csviri linked an issue Aug 21, 2023 that may be closed by this pull request
@csviri
Copy link
Contributor Author

csviri commented Aug 21, 2023

@metacosm added description.

What use case is this suppose to fix, as this looks suspicious, the parameter that is now always false had been introduced for a reason.

The initial value is always false, but otherwise changed here:

For example when the controller configuraiton is set from params:

extConfig.namespaces.map(HashSet::new).ifPresent(c::setNamespaces);

@csviri csviri requested a review from metacosm August 21, 2023 11:05
@metacosm
Copy link
Member

Then the quarkus.operator-sdk.namespaces configuration override is not working, thus setting the namespace in the annotation we should consider more like a default value.

There is no such override: quarkus.operator-sdk.namespaces is the value that is used if no other value (at
any level) is provided, it is not supposed to override values that might have been provided. That property has the lowest precedence of all properties to set namespaces. See https://docs.quarkiverse.io/quarkus-operator-sdk/dev/index.html#quarkus-operator-sdk_quarkus.operator-sdk.namespaces. Perhaps is the documentation not clear enough?

@csviri
Copy link
Contributor Author

csviri commented Aug 21, 2023

Then the quarkus.operator-sdk.namespaces configuration override is not working, thus setting the namespace in the annotation we should consider more like a default value.

There is no such override: quarkus.operator-sdk.namespaces is the value that is used if no other value (at any level) is provided, it is not supposed to override values that might have been provided. That property has the lowest precedence of all properties to set namespaces. See https://docs.quarkiverse.io/quarkus-operator-sdk/dev/index.html#quarkus-operator-sdk_quarkus.operator-sdk.namespaces. Perhaps is the documentation not clear enough?

It does not make too much sense to me this way. So IMHO should be this way (precedence order):

  1. controller configuration from property, thus: quarkus.operator-sdk.controllers."controllers".namespaces
  2. quarkus.operator-sdk.namespaces if above not set
  3. The value from annotation (if not set the default value) if above not set

Mainly it does not adds up to me, that it overrides the namespace if in the annotation the value is not set, but does not override it when it is set in the annotation. Those values should be overrideable in all cases.

@metacosm
Copy link
Member

Then the quarkus.operator-sdk.namespaces configuration override is not working, thus setting the namespace in the annotation we should consider more like a default value.

There is no such override: quarkus.operator-sdk.namespaces is the value that is used if no other value (at any level) is provided, it is not supposed to override values that might have been provided. That property has the lowest precedence of all properties to set namespaces. See https://docs.quarkiverse.io/quarkus-operator-sdk/dev/index.html#quarkus-operator-sdk_quarkus.operator-sdk.namespaces. Perhaps is the documentation not clear enough?

It does not make too much sense to me this way. So IMHO should be this way (precedence order):

1. controller configuration from property, thus: `quarkus.operator-sdk.controllers."controllers".namespaces`

2. `quarkus.operator-sdk.namespaces` if above not set

3. The value from annotation (if not set the default value) if above not set

Mainly it does not adds up to me, that it overrides the namespace if in the annotation the value is not set, but does not override it when it is set in the annotation. Those values should be overrideable in all cases.

I don't agree and that would be a breaking change. If the user made the effort to provide a value in the annotation, then it should be taken into account, not overridden by a blanket value that applies to all controllers that are not configured. This is how this property has been designed to behave.

If this is too confusing, the appropriate solution, imo, would be to remove the quarkus.operator-sdk.namespaces property altogether (which is actually something I considered).

@csviri
Copy link
Contributor Author

csviri commented Aug 21, 2023

If the user made the effort to provide a value in the annotation, then it should be taken into account, not overridden by a blanket value that applies to all controllers that are not configured. This is how this property has been designed to behave.

Maybe we should think through this for v5 of JOSDK and remove this property , not sure if the value make sense in the annotation. This really depends on that how a team deploys a controller, and it is something that naturally varies on platforms.

Basically that is why I wrote: thus setting the namespace in the annotation we should consider more like a default value.

@metacosm
Copy link
Member

If the user made the effort to provide a value in the annotation, then it should be taken into account, not overridden by a blanket value that applies to all controllers that are not configured. This is how this property has been designed to behave.

Maybe we should think through this for v5 of JOSDK and remove this property , not sure if the value make sense in the annotation. This really depends on that how a team deploys a controller, and it is something that naturally varies on platforms.

Yes, it definitely makes sense to consider cleaning things up for the next major version. I will close this PR for now, though.

@metacosm metacosm closed this Aug 21, 2023
@csviri
Copy link
Contributor Author

csviri commented Aug 21, 2023

kk, I will close the related issue

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.

The quarkus.operator-sdk.namespaces does not seems to be working
2 participants