KPO should use hook's get namespace method to get namespace#27516
Merged
dstandish merged 1 commit intoapache:mainfrom Nov 6, 2022
Merged
KPO should use hook's get namespace method to get namespace#27516dstandish merged 1 commit intoapache:mainfrom
dstandish merged 1 commit intoapache:mainfrom
Conversation
Recently hook was updated to allow non-prefixed extra fields. So we need to check non-prefixed also. Best to use the hook's "get namespace" method. But the public one has a problem; it defaults to `default`, so we don't know if it was set to that intentionally. So we use the "protected" method `_get_namespace`, which doesn't return a default. This allows us, in this case to check if we can derive from cluster, and only _then_ default to `default`.
boring-cyborg
bot
added
provider:cncf-kubernetes
2 tasks
2 tasks
bdsoha
reviewed
Nov 5, 2022
| hook_namespace = self.hook.conn_extras.get("extra__kubernetes__namespace") | ||
| # todo: replace with call to `hook.get_namespace` in 6.0, when it doesn't default to `default`. | ||
| # if namespace not actually defined in hook, we want to check k8s if in cluster | ||
| hook_namespace = self.hook._get_namespace() |
Contributor
There was a problem hiding this comment.
IMO, this method should be made public?
Contributor
Author
There was a problem hiding this comment.
yes it should. it will happen after 6.0 when we replace the current get_namespace
Contributor
There was a problem hiding this comment.
Should there be some test to validate or we already have one?
Contributor
Author
There was a problem hiding this comment.
you mean to remind us to update those in 6.0? yes, we do
Contributor
Author
There was a problem hiding this comment.
(and then when user removes _get_namespace, they'll see this reference and presumably read the todo and update it)
eladkal
approved these changes
Nov 5, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Recently hook was updated to allow non-prefixed extra fields. So we need to check non-prefixed also. Best to use the hook's "get namespace" method. But the public one has a problem; it defaults to
default, so we don't know if it was set to that intentionally. So we use the "protected" method_get_namespace, which doesn't return a default. This allows us, in this case to check if we can derive from cluster, and only then default todefault.In 6.0, get_namespace will have the correct behavior, and we'll update this again to use it instead.