feat (jkube-kit/helm) : HelmService exposes helm install functionality #3142
Conversation
This file contains 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
Eclipse JKube CI ReportStarted new GH workflow run for #3142 (2024-07-12T12:52:51Z) ⚙️ JKube E2E Tests (9907992305)
|
17 tasks
17 tasks
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
from
9a6ca43
to
491426d
Compare
17 tasks
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
from
491426d
to
347af49
Compare
18 tasks
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
2 times, most recently
from
7136585
to
f37a0cf
Compare
11 tasks
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
from
f37a0cf
to
d8af701
Compare
manusa
reviewed
Jun 21, 2024
| protected static final String PROPERTY_HELM_RELEASE_NAME = "jkube.helm.release.name"; | ||
| protected static final String PROPERTY_HELM_INSTALL_DEPENDENCY_UPDATE = "jkube.helm.install.dependencyUpdate"; | ||
| protected static final String PROPERTY_HELM_INSTALL_WAIT_READY = "jkube.helm.install.waitReady"; | ||
| protected static final String PROPERTY_HELM_INSTALL_DISABLE_OPENAPI_VALIDATION = "jkube.helm.disableOpenAPIValidation"; |
There was a problem hiding this comment.
This doesn't follow the pattern of other settings.
Is it a generic setting (i.e. it applies to more than one helm goal -install, uninstall, upgrade-)?
Or is it a setting specific to install?
I'd say it's the former, so in this case maybe the field name is not ok.
There was a problem hiding this comment.
I saw in manusa/helm-java#133 that this field is applicable for install and update, that's why I kept the name as global one.
I'll rename the field name.
rohanKanojia
commented
Jun 21, 2024
| private Path createTemporaryKubeConfigForInstall() { | ||
| if (kubernetesClient == null) { | ||
| kubernetesClient = new ClusterAccess(ClusterConfiguration.from( | ||
| System.getProperties(), jKubeConfiguration.getProject().getProperties()).build()).createDefaultClient(); |
There was a problem hiding this comment.
This is done to avoid other Helm goals requiring cluster access. When I was trying to initialize KubernetesClient in HelmService construction in JKubeServiceHub, it caused other Helm actions to start throwing exceptions in offline mode
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
from
d8af701
to
fbca00a
Compare
manusa
requested changes
Jun 26, 2024
| this(jKubeConfiguration, resourceServiceConfig, logger, null); | ||
| } | ||
|
|
||
| HelmService(JKubeConfiguration jKubeConfiguration, ResourceServiceConfig resourceServiceConfig, KitLogger logger, KubernetesClient kubernetesClient) { |
There was a problem hiding this comment.
I'm reviewing and thinking about this PR since yesterday.
The fact that we're adding extra fields to this constructor doesn't feel right (especially one that's not a configuration but a heavy KubernetesClient instance).
We should probably be able to infer the configuration using ClusterConfiguration.from(access, System.getProperties(), project.getProperties()).build();
just like we do here:
And then use the ClusterAccess#getConfig to initialize the temporary .kube/config
Maybe we can discuss this later, to see if there are better alternatives.
But storing an KubernetesClient instance here, doesn't seem right at all.
There was a problem hiding this comment.
I had added this mainly for testing. Exposing KubernetesClient field from constructor was causing other helm features to depend on K8s. It's only package private constructor, this KubernetesClient field would not be exposed via public constructor (it's passed as null)
There was a problem hiding this comment.
It doesn't matter, it's still not right to do so.
The helm module has no beef with the Kubernetes Client, it only has the dependency because the kubernetes-client dependency is high up in the hierarchy.
Let's say we want to split this in the future to allow for a helm-maven-plugin, the only dependency we'd probably want at that point would be the helm-java one, nothing else.
17 tasks
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
2 times, most recently
from
6640e57
to
e150f1c
Compare
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
3 times, most recently
from
bc06601
to
09762e3
Compare
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
2 times, most recently
from
ed551d0
to
e9887d1
Compare
Signed-off-by: Rohan Kumar <rohaan@redhat.com>
rohanKanojia
force-pushed
the
pr/issue2663-helmservice-install
branch
from
e9887d1
to
84a7a7d
Compare
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Related to #2663
prepareMockWebServerExpectationsForAggregatedDiscoveryEndpointsin KubernetesMockServerUtil for adding manual expectations for Kubernetes Aggregated Discovery endpointsinstallmethod in HelmService that would perform installation of a helm chart onto Kubernetes cluster using Helm java libraryreleaseNameinstallDependencyUpdateinstallWaitReadydisableOpenAPIValidationType of change
test, version modification, documentation, etc.)
Checklist