fix: Make AbstractOperatorExtension use the kubernetesClient if the infrastructureKubernetesClient is not set #3077
Conversation
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
would be nice to somehow verify this in an automated way, maybe using it in a test?
There was a problem hiding this comment.
Pull request overview
This PR fixes a client initialization issue in AbstractOperatorExtension where providing only a kubernetesClient (without an infrastructureKubernetesClient) would unnecessarily create a new client instance instead of reusing the provided one. The fix ensures both client references use the same instance when only one is provided, improving resource efficiency and addressing the issue discussed in the linked GitHub discussion.
Key Changes
- Modified the
infrastructureKubernetesClientinitialization to fall back tokubernetesClientbefore creating a new instance - This prevents unnecessary client creation when
kubernetesClientis provided butinfrastructureKubernetesClientis not
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| this.infrastructureKubernetesClient = | ||
| infrastructureKubernetesClient != null | ||
| ? infrastructureKubernetesClient | ||
| : new KubernetesClientBuilder().build(); | ||
| : kubernetesClient != null ? kubernetesClient : new KubernetesClientBuilder().build(); |
There was a problem hiding this comment.
The fix applied here should also be applied to ClusterDeployedOperatorExtension.Builder.build() for consistency. That builder has duplicate client initialization logic (lines 180-185) with the same issue - if only kubernetesClient is provided, it creates a new infrastructureKubernetesClient instead of reusing the provided client.
Consider applying the same ternary operator pattern:
infrastructureKubernetesClient =
infrastructureKubernetesClient != null
? infrastructureKubernetesClient
: kubernetesClient != null ? kubernetesClient : new KubernetesClientBuilder().build();
…nfrastructureKubernetesClient is not set Signed-off-by: xstefank <xstefank122@gmail.com>
xstefank
force-pushed
the
infra-client-same-as-main-if-not-set
branch
from
3c07054 to
9dba403
Compare
|
|
||
| @Test | ||
| void getAdditionalCRDsFromFiles() { | ||
| System.out.println(Path.of("").toAbsolutePath()); |
|
|
||
| import static org.junit.jupiter.api.Assertions.*; | ||
|
|
||
| class LocallyRunOperatorExtensionTest { |
There was a problem hiding this comment.
If I remember correctly this was on purpose an integration tests since we use the client there that does some initial calls on an API server
There was a problem hiding this comment.
well, it passes as unit test too so I saw no reason why to keep as IT
There was a problem hiding this comment.
yes, but it slows down the local build, especially if there is no local k8s server running
There was a problem hiding this comment.
yes, but this doesn't run long and also it seems that failsafe is not configured at all, so I think this as IT doesn't run at all
csviri
left a comment
csviri
left a comment
There was a problem hiding this comment.
OK, we can try it this way
…nfrastructureKubernetesClient is not set (#3077)
2 participants
Should help in cases like #3070