feat (jkube-kit/generator) : Add configuration option in BaseGenerator to add tags (#1188) #1462

rohanKanojia · 2022-04-20T12:26:57Z

Description

Right now there isn't a way to add tags to BuildConfiguration while
using zero config mode in generators. Adding a configuration field in
BaseGenerator which would expect comma separated tags which can be
picked up by Generator while building opinionated Image configuration

Signed-off-by: Rohan Kumar rohaan@redhat.com

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Checklist

I have read the contributing guidelines
I signed-off my commit with a user that has signed the Eclipse Contributor Agreement
I Added CHANGELOG entry
I have implemented unit tests to cover my changes
I have updated the documentation accordingly
No new bugs, code smells, etc. in SonarCloud report
I tested my code in Kubernetes
I tested my code in OpenShift

manusa · 2022-04-20T12:27:13Z

Eclipse JKube CI Report

Started new GH workflow run for #1462 (2022-05-06T14:54:51Z)

⚙️ JKube E2E Tests (2282351802)

codecov · 2022-04-20T12:47:13Z

Codecov Report

Merging #1462 (1d626c6) into master (0401297) will increase coverage by 0.00%.
The diff coverage is 91.66%.

@@            Coverage Diff            @@
##             master    #1462   +/-   ##
=========================================
  Coverage     51.34%   51.34%           
  Complexity     3812     3812           
=========================================
  Files           458      458           
  Lines         20681    20692   +11     
  Branches       2807     2808    +1     
=========================================
+ Hits          10618    10624    +6     
- Misses         8947     8952    +5     
  Partials       1116     1116

Impacted Files	Coverage Δ
...pse/jkube/generator/api/support/BaseGenerator.java	`66.37% <88.88%> (+1.56%)`	⬆️
...se/jkube/generator/javaexec/JavaExecGenerator.java	`91.66% <100.00%> (+0.07%)`	⬆️
.../eclipse/jkube/generator/karaf/KarafGenerator.java	`94.54% <100.00%> (+0.10%)`	⬆️
...clipse/jkube/generator/webapp/WebAppGenerator.java	`93.40% <100.00%> (+0.07%)`	⬆️
...it/config/service/portforward/PortForwardTask.java	`60.00% <0.00%> (-4.00%)`	⬇️
...e/jkube/kit/config/service/PortForwardService.java	`70.10% <0.00%> (-3.10%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0401297...1d626c6. Read the comment docs.

adriannowak · 2022-04-21T21:51:29Z

...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java

+    protected void addTagsFromConfig(BuildConfiguration.BuildConfigurationBuilder buildConfigurationBuilder) {
+        String commaSeparatedTags = getConfig(Config.TAGS);
+        if (StringUtils.isNotBlank(commaSeparatedTags)) {
+            String[] tags = commaSeparatedTags.split(",");


Shouldn't we trim whitespaces here?

adriannowak · 2022-04-23T07:46:47Z

...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java

@@ -227,6 +231,14 @@ protected void addLatestTagIfSnapshot(BuildConfiguration.BuildConfigurationBuild
        }
    }

+    protected void addTagsFromConfig(BuildConfiguration.BuildConfigurationBuilder buildConfigurationBuilder) {


Tags should be consistent with the requirements from https://docs.docker.com/engine/reference/commandline/tag/
A tag name must be valid ASCII and may contain lowercase and uppercase letters, digits, underscores, periods and dashes. A tag name may not start with a period or a dash and may contain a maximum of 128 characters.
Please consider adding into code and cover them with unit tests

Tag validation is currently by docker APIServer while processing tag request submitted by jkube.

Is it necessary to do validation again on our side?

manusa · 2022-05-03T10:29:24Z

jkube-kit/doc/src/main/asciidoc/inc/generator/_options_common.adoc

+
+| *tags*
+| A comma separated list of additional tags you want to tag your image with
+| `jkube.generator.tags`


Reminder to create a new Issue:

These property names are misleading, they should be renamed to:
jkube.generator.${generatorName}.tags or applicable

Then add a note indicating that the ${generatorName} placeholder should be changed

Or does it work without the generator?

The question was meant so that we tested it and create an issue, I'm not sure you did.
However, you have changed the documentation...

I'm sure that jkube.generator.from and others work when not specifying the generator name. We need to verify and make sure which properties need to be changed, and decide the approach.
Please, provide feedback before doing any action.

You're right. In BaseGenerator there is a method getConfigWithFallback which checks system and project properties with given key for config field value:

https://github.com/eclipse/jkube/blob/0401297d4bcea783a94fc178af376c7a8a5a185d/jkube-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java#L185

BaseGenerator config fields name, alias, from, fromMode, registry seems to be using getConfigWithFallback method so they seem to work with jkube.generator prefix with and without generator name.

add and tags(added in this PR) don't seems to be aligned with this approach. Right now they don't work with jkube.generator prefix and require generator name

I've reverted my doc changes and created #1489

...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java

manusa

See trimming comment

…r to add tags (eclipse-jkube#1188) Right now there isn't a way to add tags to BuildConfiguration while using zero config mode in generators. Adding a configuration field in BaseGenerator which would expect comma separated tags which can be picked up by Generator while building opinionated Image configuration Signed-off-by: Rohan Kumar <rohaan@redhat.com>

sonarcloud · 2022-05-06T14:50:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

92.9% Coverage
0.0% Duplication

rohanKanojia marked this pull request as ready for review April 20, 2022 14:49

rohanKanojia requested review from sunix and manusa and removed request for sunix April 20, 2022 14:49

adriannowak reviewed Apr 21, 2022

View reviewed changes

rohanKanojia force-pushed the pr/generator-option-tags branch from b30f5d5 to 4bd6cc9 Compare April 22, 2022 06:01

adriannowak reviewed Apr 23, 2022

View reviewed changes

manusa reviewed May 3, 2022

View reviewed changes

...e-kit/generator/api/src/main/java/org/eclipse/jkube/generator/api/support/BaseGenerator.java Outdated Show resolved Hide resolved

manusa requested changes May 3, 2022

View reviewed changes

rohanKanojia force-pushed the pr/generator-option-tags branch from 4bd6cc9 to 7beb92a Compare May 4, 2022 05:24

manusa marked this pull request as draft May 6, 2022 13:54

manusa marked this pull request as ready for review May 6, 2022 13:54

rohanKanojia force-pushed the pr/generator-option-tags branch from 7beb92a to 1d626c6 Compare May 6, 2022 14:36

rohanKanojia mentioned this pull request May 6, 2022

BaseGenerator: Verify & Align configuration options work to with properties #1489

Closed

manusa approved these changes May 6, 2022

View reviewed changes

manusa merged commit c6cca9e into eclipse-jkube:master May 6, 2022

rohanKanojia deleted the pr/generator-option-tags branch May 6, 2022 16:35

manusa added this to the 1.8.0 milestone May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat (jkube-kit/generator) : Add configuration option in BaseGenerator to add tags (#1188) #1462

feat (jkube-kit/generator) : Add configuration option in BaseGenerator to add tags (#1188) #1462

rohanKanojia commented Apr 20, 2022

manusa commented Apr 20, 2022 •

edited

Loading

codecov bot commented Apr 20, 2022 •

edited

Loading

adriannowak Apr 21, 2022 •

edited

Loading

adriannowak Apr 23, 2022 •

edited

Loading

rohanKanojia Apr 24, 2022

manusa May 3, 2022

manusa May 3, 2022

manusa May 6, 2022

rohanKanojia May 6, 2022 •

edited

Loading

rohanKanojia May 6, 2022

manusa left a comment

sonarcloud bot commented May 6, 2022

feat (jkube-kit/generator) : Add configuration option in BaseGenerator to add tags (#1188) #1462

feat (jkube-kit/generator) : Add configuration option in BaseGenerator to add tags (#1188) #1462

Conversation

rohanKanojia commented Apr 20, 2022

Description

Type of change

Checklist

manusa commented Apr 20, 2022 • edited Loading

Eclipse JKube CI Report

codecov bot commented Apr 20, 2022 • edited Loading

Codecov Report

adriannowak Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

adriannowak Apr 23, 2022 • edited Loading

Choose a reason for hiding this comment

rohanKanojia Apr 24, 2022

Choose a reason for hiding this comment

manusa May 3, 2022

Choose a reason for hiding this comment

manusa May 3, 2022

Choose a reason for hiding this comment

manusa May 6, 2022

Choose a reason for hiding this comment

rohanKanojia May 6, 2022 • edited Loading

Choose a reason for hiding this comment

rohanKanojia May 6, 2022

Choose a reason for hiding this comment

manusa left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 6, 2022

manusa commented Apr 20, 2022 •

edited

Loading

codecov bot commented Apr 20, 2022 •

edited

Loading

adriannowak Apr 21, 2022 •

edited

Loading

adriannowak Apr 23, 2022 •

edited

Loading

rohanKanojia May 6, 2022 •

edited

Loading