Add container-name configuration for kubernetes extension #24708
Conversation
Yelzhasdev
force-pushed
the
fix_#24676
branch
2 times, most recently
from
ad4d825 to
723cf7b
Compare
|
Hi thanks for this. I would prefer there was a separate commit addressing the issue that the PR is closing and another for the fix you mentioned was lost |
Hi! Done👍 |
|
Thanks. @iocanel can you please review? |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
What I see here is good.
However, I think that there is something missing.
We need to make sure that setting a custom container name does not break all other dekorators.
We should either make force this decorator to run before all other Application Container Decorators and then pass the custom container name to the rest, all to force it last. The latter seems simpler.
However, we do need to test using the custom name with at least a couple other container related changes to make sure everything plays nicely together.
| } | ||
|
|
||
| @Override | ||
| public void andThenVisit(DeploymentConfigSpecFluent<?> deploymentConfigSpecFluent, ObjectMeta objectMeta) { |
Hi! I added all decorators that extend ApplicationContainerDecorator to perform before that new decorator, also added couple of tests that change container to verify that everything is correct |
| } | ||
|
|
||
| @Override | ||
| public Class<? extends Decorator>[] after() { |
There was a problem hiding this comment.
We need to find a better way to deal with it, but there are not much we can do atm.
Resolves #24676
Add container-name configuration for kubernetes/openshift/knative to specify container name separately, backward compatible.
@geoand also after refactoring that took place in #12655 fix #11559 was lost, I added it back