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.

Sign up for GitHub

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

Jump to bottom

Allow Jib to set the base JVM image based on the target Java version #22040

Merged
merged 2 commits into from Dec 10, 2021
Merged

Allow Jib to set the base JVM image based on the target Java version #22040

merged 2 commits into from Dec 10, 2021

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 8, 2021 •
edited

@geoand geoand changed the title Jib java target Allow Jib to set the base JVM image based on the target Java version Dec 8, 2021
@quarkus-bot quarkus-bot bot added the area/core label Dec 8, 2021
@aloubyansky
Copy link
Member

Ah, it's still a draft, I approved it already :)

@geoand
Copy link
Contributor Author

geoand commented Dec 8, 2021

Thanks! It's in draft because I am waiting on input about what the proper images should be

@geoand geoand marked this pull request as ready for review December 9, 2021 09:00
@geoand
Copy link
Contributor Author

geoand commented Dec 9, 2021 •
edited

This is now a breaking change since the base JVM image was changed for JDK 11.

cc @maxandersen @cescoffier

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 9, 2021 •
edited

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 02fc0f8

Status Name Step Failures Logs Raw logs
✖ Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 11
✖ JVM Tests - JDK 17 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/opentelemetry/opentelemetry/deployment 
! Skipped: extensions/opentelemetry/opentelemetry-exporter-jaeger/deployment extensions/opentelemetry/opentelemetry-exporter-otlp/deployment integration-tests/opentelemetry and 2 more

📦 extensions/opentelemetry/opentelemetry/deployment

✖ io.quarkus.opentelemetry.deployment.RestClientOpenTelemetryTest.spanNameWithoutQueryString line 88 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a io.quarkus.opentelemetry.deployment.TestSpanExporter expected: <2> but was: <1> within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)

@gsmet
Copy link
Member

gsmet commented Dec 9, 2021

While it is a breaking change, I wonder if we should push it to 2.6 anyway.

@geoand
Copy link
Contributor Author

geoand commented Dec 9, 2021 •
edited

Yeah, I had the same thought. I don't see why not, but I'll leave it up to you

@aloubyansky
Copy link
Member

What are the chances it'll break other platform members? In general, introducing breaking changes after a CR shouldn't be acceptable.

@maxandersen
Copy link
Contributor

didnt we not also break it for the dockerfile scenario too ?

@geoand
Copy link
Contributor Author

geoand commented Dec 10, 2021

didnt we not also break it for the dockerfile scenario too ?

Not sure, I haven't seen any docker related changes

@geoand geoand merged commit 06412af into quarkusio:main Dec 10, 2021
@geoand geoand deleted the jib-java-target branch December 10, 2021 06:28
@quarkus-bot quarkus-bot bot added this to the 2.7 - main milestone Dec 10, 2021
@rsvoboda
Copy link
Member

@geoand this change could have an entry in docs.

@geoand
Copy link
Contributor Author

geoand commented Dec 13, 2021

Why do you think so? I mean the configuration property is documented and we'll add an entry to the migration guide for 2.7.

@rsvoboda
Copy link
Member

Migration guide is fine, thanks!

@geoand
Copy link
Contributor Author

geoand commented Dec 13, 2021 •
edited

The entry is here.

@rsvoboda
Copy link
Member

Great, thanks.

@geoand
Copy link
Contributor Author

geoand commented Dec 13, 2021

YW!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aloubyansky aloubyansky aloubyansky approved these changes

@cescoffier cescoffier cescoffier approved these changes

Assignees
No one assigned
Labels
area/core release/breaking-change release/noteworthy-feature
Projects
None yet
Milestone
2.7.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@geoand @aloubyansky @gsmet @maxandersen @rsvoboda @cescoffier