Skip to content
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

Kubernetes resource name and version no longer depend on container image configuration #33724

Merged

Conversation

iocanel
Copy link
Contributor

@iocanel iocanel commented May 31, 2023

Resolves: #33554

@iocanel iocanel force-pushed the fix-33554-k8s-defaulting-container-img-name branch from 4974bd9 to 6f9f0ee Compare June 14, 2023 16:16
@iocanel iocanel force-pushed the fix-33554-k8s-defaulting-container-img-name branch from 6f9f0ee to fcb6b3a Compare June 14, 2023 16:29
@iocanel iocanel marked this pull request as ready for review June 14, 2023 16:30
@iocanel iocanel requested a review from Sgitario June 14, 2023 16:30
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 14, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in ApplyOpenshiftRouteConfigurator.java is a change of behavior that needs to be justified (from my point of view, if the route is exposed, why should we edit a provided one by the user?).

I think we should first fully understand why the failing test is not working well after the changes in the first commit.

@iocanel
Copy link
Contributor Author

iocanel commented Jun 15, 2023

The changes in ApplyOpenshiftRouteConfigurator.java is a change of behavior that needs to be justified (from my point of view, if the route is exposed, why should we edit a provided one by the user?).

Generally speaking, the whole idea of using user provided resources is to further enhance / dekorate them using the info gathered during the build.

This is also the behavior that is tested by the OpenshiftWithCustomRouteResource test: User provides a Route without a port and the test expects to find one.

I think we should first fully understand why the failing test is not working well after the changes in the first commit.

By removing the defaults we end up with 0 openshift properties and therefore we trigger: dekorateio/dekorate#1215. The side effect is that dekorate generates a RouteConfig without a target port.

Regardless, of what dekorate does internally, quarkus does have a targetPort with a default value http and this should be respected. Currently, it is not because we only apply that when expose == true. As explained above this is not the expected behavior something that is also reinforced by the logic of OpenshiftWithCustomRouteResource

@Sgitario
Copy link
Contributor

The changes in ApplyOpenshiftRouteConfigurator.java is a change of behavior that needs to be justified (from my point of view, if the route is exposed, why should we edit a provided one by the user?).

Generally speaking, the whole idea of using user provided resources is to further enhance / dekorate them using the info gathered during the build.

This is also the behavior that is tested by the OpenshiftWithCustomRouteResource test: User provides a Route without a port and the test expects to find one.

I think we should first fully understand why the failing test is not working well after the changes in the first commit.

By removing the defaults we end up with 0 openshift properties and therefore we trigger: dekorateio/dekorate#1215. The side effect is that dekorate generates a RouteConfig without a target port.

Regardless, of what dekorate does internally, quarkus does have a targetPort with a default value http and this should be respected. Currently, it is not because we only apply that when expose == true. As explained above this is not the expected behavior something that is also reinforced by the logic of OpenshiftWithCustomRouteResource

So, before your changes, the targetPort was set to http by Dekorate, not by Quarkus.
Now, because of this issue in Dekorate, Dekorate does not play anymore. Since Quarkus was doing nothing already, the target port remained untouched.

With your changes about the expose == true, now Quarkus does set the target port (regardless if Dekorate does something or not).

Did I understand what is going well now?

@Sgitario Sgitario self-requested a review June 15, 2023 09:59
Copy link
Contributor

@Sgitario Sgitario left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@iocanel iocanel merged commit 13c768b into quarkusio:main Jun 15, 2023
@quarkus-bot quarkus-bot bot added this to the 3.2 - main milestone Jun 15, 2023
@rsvoboda
Copy link
Member

So what's the right way to customize the names in kubernetes yaml? Is it quarkus.application.name?

I mean these names:

yq '.spec.template.spec.containers[].name' target/kubernetes/kubernetes.yml
yq '.metadata.name' target/kubernetes/kubernetes.yml

@rsvoboda
Copy link
Member

@iocanel @Sgitario @cescoffier ^^^

@iocanel
Copy link
Contributor Author

iocanel commented Aug 1, 2023

@rsvoboda: quarkus.kubernetes.name is still the way to go. The PR does not change that.

@rsvoboda
Copy link
Member

rsvoboda commented Aug 1, 2023

Thanks @iocanel! I checked the docs and it's mentioned in deploying-to-kubernetes.adoc

docs/src/main/asciidoc/deploying-to-kubernetes.adoc:The name of the resource is determined by the application name and may be overridden by quarkus.kubernetes.name, quarkus.openshift.name and quarkus.knative.name.

tbh I didn't find the info in https://quarkus.io/guides/deploying-to-kubernetes as I gave up. The guide is super long, docs/src/main/asciidoc/deploying-to-kubernetes.adoc has almost 1700 lines + huge number of config properties.

The only idea I have atm is to have the config properties listed in separate document and reference it from deploying-to-kubernetes guide.

curl https://quarkus.io/guides/deploying-to-kubernetes 2>/dev/null | html2text | wc -l 
    9594

I will open dedicated issue to collect ideas.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubernetes name influenced by the quarkus.container-image.name
3 participants