-
Notifications
You must be signed in to change notification settings - Fork 528
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
Provide a way to set labels on images defined by Generators #2885
Comments
@davidecavestro : Would it be possible for you to work on this? I'm happy to provide you code pointers if interested. |
Not 100% sure how to do it. Maybe adding a map/property configuration option in Mojos/ Gradle tasks can help achieve this. But then maybe we would need to align that configuration option with both generators (zero configuration) and explicit image configuration. |
I see, not sure if filter support could help someway... should investigate. |
I think the easiest way right now would be to control that from Maven by computing the value as a property first: <properties>
<prop>pre</prop>
<jkube.generator.labels>${prop}-${env.ENV_PROP}</jkube.generator.labels>
<!-- ... -->
</properties>
Not sure of how we're doing that for tags now, but both features should follow the same approach for sanitizing inputs. |
so I see the interpolation is done by maven both defining a property and directly using expressions such as
I see no sanitization for tags at addTagsFromConfig OTOH EnvUtils.splitOnSpaceWithEscape could be replicated into separate methods to extract lists (tags use case) and maps (labels use case) supporting the escape of both comma chars (entries separators) and equal chars (key/value separators). That said, it may seem too complicated, as it reduces code readability increasing complexity. |
Yes, that should work too.
For Gradle it should be done differently, but would be achievable too. What I mean, in general, is that for dynamic values instead of reinventing the wheel downstream, it's better to leverage the tools that the build system offers.
Maybe in this case (it seems to be far more complex than for tags) it's better to try and see what Docker or other tools are doing to ensure a consistent behavior. https://docs.docker.com/config/labels-custom-metadata/#label-keys-and-values |
Please note that the complexity of parsing and escaping separators is clearly related to the choice of setting labels via a CSV (of key value pairs). Also consider that in case we opt instead for shaping labels with nested config elements, we would end up with something simpler; but this approach would differ from tags one (that's why I started with the former). That is (for maven) <labels>foo=abc,bar=123</labels> VS <labels>
<foo>abc</foo>
<bar>123</bar>
</labels> |
OK, note that we would likely want to support the label setting via configuration elements (regardless of the nesting choice), but also through the use of a property |
makes sense |
What about json? |
@davidecavestro : Could you please open a pull request with your current changes that is covering the simple scenario? Maybe we can iterate over it to add more complex expressions? |
After some issues with GPG signature (ssh keys were not an option on this network) I saw some issues on tests and licenses check: if they are not false positives I can look at them |
) (2956) feat: provide a way to set labels on images defined by Generators (#2885) --- doc: update CHANGELOG.md
Component
Kubernetes Maven Plugin
Is your enhancement related to a problem? Please describe
I miss a way to set labels on the container image when I build it through generators (instead of the images config). I use the spring-boot generator, but I suppose the issue is common to others.
Describe the solution you'd like
I'd like to add a labels entry to the generator config, such as
Probably a comma separated list of values would resemble what already happens for jkube.generator.tags.
Describe alternatives you've considered
Using images tag or dockerfile I would loose the support for automatic detection of layered jars
Additional context
Discussed at #2884
The text was updated successfully, but these errors were encountered: