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: runtime namespaces are now correctly set from controller properties #726

Merged
merged 1 commit into from
Sep 24, 2023

Conversation

jcechace
Copy link
Contributor

Fixes #725

@metacosm If there are soem tests which I can draw inspiration from, let me know and I will add some coverage. Otherwise I will be leaving this up to you as understanding how exactly I should write the tests is something I don't have the time for right now.

@jcechace
Copy link
Contributor Author

@metacosm This PR respects the precendence order of controller level prop -> annotation -> opertor level (blanket) prop. However I still think that such priority of evaluation is illogical and inherently flawed for runtime use. There are several reasons why

  1. Annotation is a build time configuration by nature, it doesn't make sense that a runtime property (albeit a global / blanekt one) has lower precendence
  2. Annotation value (being a build time config) is hidden from the actual person deploying/using the operator. It taking precendence over something explicitly visible (such as QUARKUS_OPERATOR_SDK_NAMESPACES) is IMHO extremely confusing.

Personally I would see the order ascontroller level prop -> opertor level (blanket) prop -> annotation.

The reasoning in this case is that QUARKUS_OPERATOR_SDK_NAMESPACES is equivalent to setting QUARKUS_OPERATOR_SDK_CONTROLLERS_<name>_NAMESPACES for each controller. The ability to override the setting for specific controller on runtime is of course kept.

@jcechace jcechace changed the title runtime namespaces are now correctly set from controller properties fix: runtime namespaces are now correctly set from controller properties Sep 23, 2023
@metacosm
Copy link
Member

@metacosm This PR respects the precendence order of controller level prop -> annotation -> opertor level (blanket) prop. However I still think that such priority of evaluation is illogical and inherently flawed for runtime use. There are several reasons why

1. Annotation is a build time configuration by nature, it doesn't make sense that a runtime property (albeit a global / blanekt one) has lower precendence

2. Annotation value  (being a build time config) is hidden from the actual person deploying/using the operator. It taking precendence over something explicitly visible (such as `QUARKUS_OPERATOR_SDK_NAMESPACES`) is IMHO extremely confusing.

Personally I would see the order ascontroller level prop -> opertor level (blanket) prop -> annotation.

The reasoning in this case is that QUARKUS_OPERATOR_SDK_NAMESPACES is equivalent to setting QUARKUS_OPERATOR_SDK_CONTROLLERS_<name>_NAMESPACES for each controller. The ability to override the setting for specific controller on runtime is of course kept.

Yes, this makes sense, however I will not make this breaking change (though you could argue that this is currently broken anyway) in a patch release. We will change this behavior to follow the one you describe in the next minor release.

@jcechace
Copy link
Contributor Author

@metacosm This PR respects the precendence order of controller level prop -> annotation -> opertor level (blanket) prop. However I still think that such priority of evaluation is illogical and inherently flawed for runtime use. There are several reasons why

1. Annotation is a build time configuration by nature, it doesn't make sense that a runtime property (albeit a global / blanekt one) has lower precendence

2. Annotation value  (being a build time config) is hidden from the actual person deploying/using the operator. It taking precendence over something explicitly visible (such as `QUARKUS_OPERATOR_SDK_NAMESPACES`) is IMHO extremely confusing.

Personally I would see the order ascontroller level prop -> opertor level (blanket) prop -> annotation.
The reasoning in this case is that QUARKUS_OPERATOR_SDK_NAMESPACES is equivalent to setting QUARKUS_OPERATOR_SDK_CONTROLLERS_<name>_NAMESPACES for each controller. The ability to override the setting for specific controller on runtime is of course kept.

Yes, this makes sense, however I will not make this breaking change (though you could argue that this is currently broken anyway) in a patch release. We will change this behavior to follow the one you describe in the next minor release.

Sure thing -- that was the primary reason for not including it here. I can follow up with another PR later.

@metacosm metacosm merged commit 6683f01 into quarkiverse:6.3.x Sep 24, 2023
3 checks passed
@metacosm
Copy link
Member

Merging this, will change the behavior in the next minor and add tests there.

@jcechace
Copy link
Contributor Author

@metacosm I think this should have also gotten into 6.4 and 6.5 (so main maybe?)

@metacosm
Copy link
Member

metacosm commented Dec 12, 2023

@metacosm I think this should have also gotten into 6.4 and 6.5 (so main maybe?)

Indeed, I never got around to making the changes I initially intended so will add this change to main as well. Opened #784 to address this.

metacosm added a commit that referenced this pull request Dec 12, 2023
…ies (#726) (#784)

Fixes #725

Co-authored-by: Jakub Cechacek <jcechace@gmail.com>
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.

None yet

2 participants