Introduce AbstractHelmMojo class #2714
Merged
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
When new Helm goals will be written, this class will serve as a base class. Previously, the HelmMojo class served as both a Helm goal and a base class for other Helm goals (like helm-push and helm-lint). Signed-off-by: Jurrie Overgoor <[email protected]>
Eclipse JKube CI ReportStarted new GH workflow run for #2714 (2024-03-04T20:44:16Z) ⚙️ JKube E2E Tests (8146596588)
|
Jurrie
commented
Feb 21, 2024
...in/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmLintMojo.java
Show resolved
Hide resolved
Jurrie
commented
Feb 21, 2024
...in/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmPushMojo.java
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2714 /- ##
=============================================
Coverage 59.36% 70.52% 11.15%
- Complexity 4586 5007 421
=============================================
Files 500 487 -13
Lines 21211 19461 -1750
Branches 2830 2504 -326
=============================================
Hits 12591 13724 1133
Misses 7370 4514 -2856
Partials 1250 1223 -27 ☔ View full report in Codecov by Sentry. |
...in/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmPushMojo.java
Show resolved
Hide resolved
...plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/OpenshiftHelmMojo.java
Outdated
Show resolved
Hide resolved
...aven-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmPushMojo.java
Show resolved
Hide resolved
manusa
reviewed
Feb 22, 2024
Comment on lines
56
to
58
| protected String getLogPrefix() { | ||
| return OpenShift.DEFAULT_LOG_PREFIX; | ||
| } |
There was a problem hiding this comment.
The OpenShift overrides are required to preserve the OpenShift-specific behavior (in this case the logged statements should be prefixed with oc: as opposed to k8s: same happens with the other apparently redundant overrides.
There was a problem hiding this comment.
Ah, I missed that this reads OpenShift :) I reverted this change in c036268.
There was a problem hiding this comment.
As for the other apparently redundant overrides: I feel that they are actually redundant. (#2714 (comment) for example.) But of course I might be wrong 😀
There was a problem hiding this comment.
Basically there are 4 things to override for the OpenShift Helm experience.
- Static literal for the logger
- Default enum value for the helm type
getKubernetesManifest: should respond to the user-provided property or configurationopenShiftManifestinstead- getKubernetesTemplate: should respond to the user-provided property or configuration
openshiftTemplateinstead (note that there is a bug in the current OpenshiftHelmPushMojo -Already taken care of pull/2714/files#r1498286179-) in the property, it reads"jkube.kubernetesManifest"when it should be `"jkube.openshiftTemplate""
As for the last two, I assume they were introduced to be able to override the default value of the parameter.
The only one redundant or unnecessary for the Helm Push mojo is the one for the kubernetesManifest, which is used for the warning log. (As a user, I want to get warned if I run the helm goal without having run the resource goal before, so that I'm aware of my misconfiguration). However, this warning should also apply for the Lint mojo.
The new AbstractHelmMojo has kubernetesTemplate, so this override should be kept.
There was a problem hiding this comment.
This holds for all OpenShift Helm classes that are changed, so for:
- OpenshiftHelmLintMojo
- OpenshiftHelmMojo
- OpenshiftHelmPushMojo
Let's be methodical and list per method-to-be-overridden here:
getKubernetesManifest()
- OpenshiftHelmLintMojo: not available in parent classes; can't override
- OpenshiftHelmMojo: overridden on line 44: https://github.com/eclipse/jkube/pull/2714/files#diff-6290a43bd73acd81a35c9ece497c1315ed7e146c5e52a39887ccda6f40c29868R44
- OpenshiftHelmPushMojo: not available in parent classes; can't override
The getKubernetesManifest() method is defined in org.eclipse.jkube.maven.plugin.mojo.build.HelmMojo on line 70: https://github.com/eclipse/jkube/pull/2714/files#diff-1e94b9a102c6e2c667d4ee4da8631008a2237cb933ceff7bb0d365fe945b6fc0R70. Because it's available in HelmMojo, it can be overridden in OpenshiftHelmMojo.
Put differently; because it is not available in org.eclipse.jkube.maven.plugin.mojo.build.AbstractHelmMojo, it can not be overridden in HelmLintMojo or in OpenshiftHelmLintMojo, and it can not be overridden in HelmPushMojo or in OpenshiftHelmPushMojo.
The Kubernetes manifest property is only used in HelmMojo to generate a warning when k8s:resource or oc:resource is not ran. Do you think that the same warning should be shown when k8s:helm-lint, oc:helm-lint, k8s:helm-push or oc:helm-push is ran?
getKubernetesTemplate()
- OpenshiftHelmLintMojo: overridden on line 40: https://github.com/eclipse/jkube/pull/2714/files#diff-814cb60fbdb969f6117757d510d1ee3502387b6cce13fd4bd654f9014ee195a6R40
- OpenshiftHelmMojo: overridden on line 50: https://github.com/eclipse/jkube/pull/2714/files#diff-6290a43bd73acd81a35c9ece497c1315ed7e146c5e52a39887ccda6f40c29868R50
- OpenshiftHelmPushMojo: overridden on line 39: https://github.com/eclipse/jkube/pull/2714/files#diff-e8f5cf8fb0ab049afb61be5e7a00aacefd5bbc68009345c6ba1e86806eecc4b2R39
getDefaultHelmType()
- OpenshiftHelmLintMojo: overridden on line 45: https://github.com/eclipse/jkube/pull/2714/files#diff-814cb60fbdb969f6117757d510d1ee3502387b6cce13fd4bd654f9014ee195a6R45
- OpenshiftHelmMojo: overridden on line 54: https://github.com/eclipse/jkube/pull/2714/files#diff-6290a43bd73acd81a35c9ece497c1315ed7e146c5e52a39887ccda6f40c29868R54
- OpenshiftHelmPushMojo: overridden on line 44: https://github.com/eclipse/jkube/pull/2714/files#diff-e8f5cf8fb0ab049afb61be5e7a00aacefd5bbc68009345c6ba1e86806eecc4b2R44
getLogPrefix()
- OpenshiftHelmLintMojo: overridden on line 50: https://github.com/eclipse/jkube/pull/2714/files#diff-814cb60fbdb969f6117757d510d1ee3502387b6cce13fd4bd654f9014ee195a6R50
- OpenshiftHelmMojo: overridden on line 59: https://github.com/eclipse/jkube/pull/2714/files#diff-6290a43bd73acd81a35c9ece497c1315ed7e146c5e52a39887ccda6f40c29868R59
- OpenshiftHelmPushMojo: overridden on line 49: https://github.com/eclipse/jkube/pull/2714/files#diff-e8f5cf8fb0ab049afb61be5e7a00aacefd5bbc68009345c6ba1e86806eecc4b2R49
There was a problem hiding this comment.
This sounds about right.
The Kubernetes manifest property is only used in
HelmMojoto generate a warning whenk8s:resourceoroc:resourceis not ran. Do you think that the same warning should be shown whenk8s:helm-lint,oc:helm-lint,k8s:helm-pushoroc:helm-pushis ran?
This is necessary for those goals that depend on the resources. So only for the initial HelmMojo. However, for the others, it'd be nice to have a similar warning reminding user to run the helm goal first (follow-up PR), the Gradle task behavior should be aligned too (follow-up PR 2).
Signed-off-by: Jurrie Overgoor <[email protected]>
Jurrie
commented
Feb 22, 2024
...es-maven-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmMojo.java
Show resolved
Hide resolved
Jurrie
commented
Feb 22, 2024
...es-maven-plugin/plugin/src/main/java/org/eclipse/jkube/maven/plugin/mojo/build/HelmMojo.java
Show resolved
Hide resolved
|
Some E2E integration tests seem to be failing HelmConfigITCase.k8sHelmPush: |
@rohanKanojia Interesting! How do I run these integration tests locally? Just |
|
@Jurrie : I think you can run springboot suite via |
@rohanKanojia That does not seem to work: |
|
@Jurrie : Where are you running this command? You need to clone jkube-integration-tests repository and run it from there. |
Oh my, the integration tests are actually in another repository of a different organization. Did I miss that somewhere in the documentation, or is that not documented yet? 😀 Anyway, I know why the integration tests are failing: the To me this seems like a bug, as the documentation states: "The k8s:resource and the k8s:helm goals are required to create the resource descriptors which are included in the Helm chart and the Helm chart itself. If you have already built the resource and created the chart, then you can omit these goals." Previously this worked because the would actually run the How do I proceed? Is the documentation wrong, and is the Update: I think this would be fixed by merging PR eclipse-jkube/jkube-integration-tests#365, but to be honest: it's more a side-effect than an intended change 😀 |
Need to look into this, I can't remember at the moment if this was intentional or not (probably was). Note that both these features are legacy from the Fabric8 Maven Plugin that were later improved when migrated to this repo. The documentation wasn't very precise at the moment, and there was no written spec either. We'll give you a reply by Monday (today is an internal holiday at Red Hat). |
Yes, it is a bug indeed. Documentation clearly states the requirement to manually run the As a note for maintainers: The helm-push goal was not available for the fabric8-maven-plugin
The code is wrong |
|
rohanKanojia
approved these changes
Mar 5, 2024
Please see #2772.
I'm afraid I'm not using Gradle, so I think I'm not the best person the do this. |
@Jurrie : Could you please open an issue for this task? Maybe some community member can port your fix to gradle plugins. |
arsenalzp
pushed a commit
to arsenalzp/jkube
that referenced
this pull request
Mar 12, 2024
* Introduce AbstractHelmMojo class When new Helm goals will be written, this class will serve as a base class. Previously, the HelmMojo class served as both a Helm goal and a base class for other Helm goals (like helm-push and helm-lint). * Implement pull request comments * Add a quick note on the separate integration tests repository
2 tasks
17 tasks
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
When new Helm goals will be written, this class will serve as a base class. (I'd like to take a stab at introducing
helm-installandhelm-uninstallgoals.) Previously, the HelmMojo class served as both a Helm goal and a base class for other Helm goals (like helm-push and helm-lint).Type of change
test, version modification, documentation, etc.)
Checklist