Ensure that the name property of @ComponentApplication is applied #281
Conversation
geoand
changed the title
...mponent-annotations/src/it/issue-275/src/main/java/io/dekorate/issue275/DemoApplication.java
Outdated
Show resolved
Hide resolved
...ions/component-annotations/src/main/java/io/dekorate/component/handler/ComponentHandler.java
Outdated
Show resolved
Hide resolved
|
On hold for the time being |
geoand
changed the title
|
Ready for new review |
geoand
changed the title
| @@ -150,10 +152,12 @@ private BaseConfig getKubernetesConfig() { | |||
| * @return The component. | |||
| */ | |||
| private Component createComponent(ComponentConfig config) { | |||
| return new ComponentBuilder() | |||
| Map<String, String> labels = resources.getLabels(); | |||
| labels.put(Labels.APP, config.getName()); | |||
There was a problem hiding this comment.
LGTM but I'm just wondering if it should be done in a decorator? @iocanel
There was a problem hiding this comment.
I prefer to have as many things as possibly set via decorator, but it's not a blocker for me.
The benefit of using a decoratror is that you don't have to keep track of the origin.
Was the resource created using fallback config? It works.
Was the resource created externally? it works.
There was a problem hiding this comment.
Makes sense. Does it apply to this case though?
There was a problem hiding this comment.
For the name and labels not, as we don't do that in the rest of the modules.
|
Overall, this seems to me like another side-effect of having the ComponentHandler to run without KuberenetesConfig, which leads to no-one setting the application info. I am wondering if we could just resolve that using the handler @geoand created. and just do a setApplicationInfo() inside handle like the KubernetesHanlder does. |
|
@iocanel I don't think that is the case. I think here the case is that for |
|
@geoand ok lets not hold the PR more, just make all constatnts in Labels public to keep the symetry and lets merge. |
|
@iocanel done, thanks |
Fixes: #276