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

-Dquarkus.http.host=0.0.0.0 force set in s2i and jib despite it being the default #11911

Closed
maxandersen opened this issue Sep 4, 2020 · 7 comments · Fixed by #11918
Closed
Labels
area/container-image kind/bug Something isn't working
Milestone

Comments

@maxandersen
Copy link
Contributor

Describe the bug
just noticed that -Dquarkus.http.host=0.0.0.0 being set on various jvm/native arguments on https://quarkus.io/guides/all-config for s2i and jib.

Since #1307 that been the default value anyway and it also means any setting in application.properties will get ignored (afaics).

Is that really the intention ?

I would have expected default values to be used or what set in application.properties. I wonder if it was just done this way as the default was not set to make it work out of box back then....consequence now is though if we change it we do run the risk of breaking users since i their quarkus.http.host is set in application.properties - it would have been ignored by now for s2i and jib usecases.

We should though fix this imo (assuming there are no good reason we force them) and document it clearly.

@maxandersen maxandersen added the kind/bug Something isn't working label Sep 4, 2020
@quarkusbot
Copy link

/cc @geoand

@geoand
Copy link
Contributor

geoand commented Sep 4, 2020

I also found it weird as well... We should definitely get rid of it as it's the default which is very unlikely to change at this point

@maxandersen
Copy link
Contributor Author

well even it was to change; isn't ones expectation that settings in application.properties should be listened too ?

@geoand
Copy link
Contributor

geoand commented Sep 4, 2020

Yes for sure and this setting does indeed violate that expectation (although in all this time no one I have seen has tried to set quarkus.http.host and run into this - probably because no one is setting quarkus.http.host :)).

@maxandersen
Copy link
Contributor Author

yeah, very likely and thus just something to clean up to not trigger my ocd for clean settings ;)

@geoand
Copy link
Contributor

geoand commented Sep 4, 2020

I'll do this over the weekend if you don't beat me to it :)

geoand added a commit to geoand/quarkus that referenced this issue Sep 5, 2020
There is no need to set these to 0.0.0.0 as that value
is the default. Furthermore setting them in such a way
means that if the property is set in application.properties,
it will have no effect when running the container image

Fixes: quarkusio#11911
@geoand
Copy link
Contributor

geoand commented Sep 5, 2020

#11918 takes care of this, but I am thinking we probably want to fix it for docker as well

geoand added a commit to geoand/quarkus that referenced this issue Sep 5, 2020
There is no need to set these to 0.0.0.0 as that value
is the default. Furthermore setting them in such a way
means that if the property is set in application.properties,
it will have no effect when running the container image

Fixes: quarkusio#11911
geoand added a commit that referenced this issue Sep 7, 2020
Drop quarkus.http.host setting from Jib and S2I
@gsmet gsmet added this to the 1.9.0 - master milestone Sep 8, 2020
gsmet pushed a commit to gsmet/quarkus that referenced this issue Sep 8, 2020
There is no need to set these to 0.0.0.0 as that value
is the default. Furthermore setting them in such a way
means that if the property is set in application.properties,
it will have no effect when running the container image

Fixes: quarkusio#11911
@gsmet gsmet modified the milestones: 1.9.0 - master, 1.8.0.Final Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/container-image kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants