-
Notifications
You must be signed in to change notification settings - Fork 496
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
refactor: remove ClusterAccess wrapper class #3200
refactor: remove ClusterAccess wrapper class #3200
Conversation
Eclipse JKube CI ReportStarted new GH workflow run for #3200 (2024-07-11T06:40:54Z) ⚙️ JKube E2E Tests (9886579666)
|
@@ -38,6 +39,7 @@ public class JKubeConfiguration implements Serializable { | |||
private static final long serialVersionUID = 7459084747241070651L; | |||
|
|||
private JavaProject project; | |||
private transient ClusterConfiguration clusterConfiguration; |
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.
Why transient?
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.
IDE was showing a warning as ClusterConfiguration is not serializable . I think we do not want to expose this field via Serialization
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.
ClusterConfiguration can be serializable, just implement it and add the serial version.
The entire hierarchy of JKubeCofiguration should be subject to serialization.
I don't mind if it's in this PR or in a follow up PR, but ClusterConfiguration->ClusterAccess should be removed from JKubeServiceHub constructor since now it's part of JKubeConfiguration |
5677669
to
b474406
Compare
…common` + Move ClusterAccess and ClusterConfiguration classes from `jkube-kit/config/resource` -> `jkube-kit/common` so that they're accessible to JKubeConfiguration + Add new field `clusterConfiguration` in JKubeConfiguration for passing in Cluster configuration + Add new field `currentContext` to ClusterConfiguration in order to pass current context Signed-off-by: Rohan Kumar <rohaan@redhat.com>
ClusterAccess is not required in the constructor as ClusterConfiguration is being passed in JKubeConfiguration Signed-off-by: Rohan Kumar <rohaan@redhat.com>
b474406
to
d97cba2
Compare
@@ -176,7 +174,7 @@ protected GeneratorContext.GeneratorContextBuilder initGeneratorContextBuilder() | |||
.prePackagePhase(false) | |||
.useProjectClasspath(kubernetesExtension.getUseProjectClassPathOrDefault()) | |||
.sourceDirectory(kubernetesExtension.getBuildSourceDirectoryOrDefault()) | |||
.openshiftNamespace(StringUtils.isNotBlank(kubernetesExtension.getNamespaceOrNull()) ? kubernetesExtension.getNamespaceOrNull() : clusterAccess.getNamespace()) | |||
.openshiftNamespace(StringUtils.isNotBlank(kubernetesExtension.getNamespaceOrNull()) ? kubernetesExtension.getNamespaceOrNull() : new ClusterAccess(initClusterConfiguration()).getNamespace()) |
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.
Why do we need ClusterAccess here?
Have you checked what ClusterAccess.getNamespace does?
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 think we're not on the same page here.
It's basically easier to try and tackle #2164 first and then do whatever is needed here.
If you carefully analyze what you're doing at the moment is just adding more wrappers around clusterconfiguration, so basically you're making things worse.
If you simply remove ClusterAccess
from the picture you'll see that everything falls naturally into place.
Do you prefer for me to bootstrap these changes?
176dfd6
to
167889e
Compare
ClusterAccess is just a wrapper around KubernetesClient. Remove it and directly use KubernetesClient and ClusterConfiguration wherever applicable. Signed-off-by: Rohan Kumar <rohaan@redhat.com>
167889e
to
39a9115
Compare
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.
jkube-kit/common
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Quality Gate failedFailed conditions |
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, thx!
Description
Fix #2164
Related to #3142
jkube-kit/config/resource
->jkube-kit/common
so that they're accessible to JKubeConfigurationclusterConfiguration
in JKubeConfiguration for passing in Cluster configurationcurrentContext
to ClusterConfiguration in order to pass current contextis being passed in JKubeConfiguration
Type of change
test, version modification, documentation, etc.)
Checklist