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

Build google-cloud-functions-http module with Java 8. #9912

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

gemmellr
Copy link
Contributor

@gemmellr gemmellr commented Jun 10, 2020

Alternative to #9908 (which does what I originally proposed doing / had also done but not raised yet) for discussion.

Fix for #9904.

@boring-cyborg boring-cyborg bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Jun 10, 2020
@gemmellr gemmellr changed the title Build google-cloud-functions-http module with Java 8. Reverts CI config changes… Build google-cloud-functions-http module with Java 8. Jun 10, 2020
@loicmathieu
Copy link
Contributor

Please, keep these integration tests excluded from the CI Jobs as there is no point to test on Java 8 something that only runs on 11.

Notice also that Java 8 is the default source/target for integration tests, so removing the properties is enought.

@gemmellr
Copy link
Contributor Author

Im on the fence about the exclusion. On the one hand, yes those tests arent doing much of anything on 8. However that also means theres little reason to bother complicating the CI config to exclude the module. I think having the module excluded in CI largely just makes it more likely for the Java 8 build to be broken again without people noticing (much as it was to cause all this discussion), especially if you do plan to add automated tests that then require 11 to run and dont prevent them running on 8.

Agreed on the properties, I just pushed what I had played with as experiment altering the earlier profile fix branch that I decided not to raise a PR for. Should have thought further on that bit.

@gemmellr
Copy link
Contributor Author

I pushed a new version that jsut removes the compiler properties as thats the main issue and can be updated by itself. I still think the CI exclusions should probably be removed too though.

Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM.

If someone else think it's a good idea to remove the CI exclusion, I'm OK with it.

@geoand
Copy link
Contributor

geoand commented Jun 11, 2020

I'll merge this since we have various CI failures caused by the Java 11 requirement

@geoand geoand merged commit 790cd68 into quarkusio:master Jun 11, 2020
@gsmet gsmet added this to the 1.6.0 - master milestone Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants