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

Run Quarkus Quickstarts with Quarkus PRs #26805

Merged
merged 1 commit into from Aug 3, 2022

Conversation

rsvoboda
Copy link
Member

@rsvoboda rsvoboda commented Jul 19, 2022

Run Quarkus Quickstarts (QS) with Quarkus PRs, QS executed in JVM mode

There were several cases when QS got broken after changes in main. This will give early warning before PR gets in.

Ecosystem CI for QS provides info when things fail, the response loop is often quite long and people do not monitor that actively.

@quarkus-bot quarkus-bot bot added the area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure label Jul 19, 2022
@gsmet gsmet changed the title Run Quarkus Quickstars with Quarkus PRs Run Quarkus Quickstarts with Quarkus PRs Jul 19, 2022
@rsvoboda
Copy link
Member Author

[INFO] Running org.acme.quickstart.oidc.CodeFlowTest
Downloading from google-maven-central: https://maven-central.storage-download.googleapis.com/maven2/org/keycloak/keycloak-core/17.0.0-legacy/keycloak-core-17.0.0-legacy.pom
Downloading from central: https://repo.maven.apache.org/maven2/org/keycloak/keycloak-core/17.0.0-legacy/keycloak-core-17.0.0-legacy.pom
Downloading from google-maven-central: https://maven-central.storage-download.googleapis.com/maven2/org/keycloak/keycloak-core/17.0.0-legacy/keycloak-core-17.0.0-legacy.jar
Downloading from central: https://repo.maven.apache.org/maven2/org/keycloak/keycloak-core/17.0.0-legacy/keycloak-core-17.0.0-legacy.jar

This is really strange, security-openid-connect-multi-tenancy-quickstart defines <keycloak.version>17.0.0-legacy</keycloak.version> property for the docker image. But somehow the value of the property gets used to pull the keycloak-core artifact of the same version.
As if the bom/application/pom.xml: <keycloak.version>18.0.2</keycloak.version> from Quarkus get's replaced by the property defined in the QS.
The strangest thing is that the behaviour can be reproduced here, but not on my local machine or in other envs.

@sberyozkin / @gsmet any tips?

I'm thinking of changing the property name in QS from keycloak.version to keycloak.image.version

@sberyozkin
Copy link
Member

Thanks @rsvoboda for opening quarkusio/quarkus-quickstarts#1144

@rsvoboda
Copy link
Member Author

Quickstarts Tests are green, failure in module not related to this change

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@famod
Copy link
Member

famod commented Jul 21, 2022

So, with this change even the tiniest change will run run two jobs that take 40m each?
Bit of a step back in terms of PR build efficiency. But I get the point of having an earlier hint.

@rsvoboda
Copy link
Member Author

Yup, don't have many ideas for optimizations. Not sure if we can easily map impacted_modules to Quickstarts modules.

We could probably add -DskipTests to speed up QS workflows and ensure just the compilation.

@famod
Copy link
Member

famod commented Jul 22, 2022

Yup, don't have many ideas for optimizations. Not sure if we can easily map impacted_modules to Quickstarts modules.

I've been thinking about extending GIB to take in a list of changed artifacts that it then matches to the dependencies of each QS module and if no such dependency is present, excludes the QS module.
Two challenges with that:

  1. I don't have time ATM to work on that.
  2. We'd need some "magic" to cover those "dreaded" deployment artifacts (that are not declared in QS pom.xmls).

Mapping the impacted_modules manually might be doable but would be challenging to maintain.

We could probably add -DskipTests to speed up QS workflows and ensure just the compilation.

If that still meets the minimum quality goals then 👍 .
Additionally/alternatively we could try -T2, but this might not be any faster, given how weak CI runners are and it also requires random ports (ideally).

@famod
Copy link
Member

famod commented Jul 22, 2022

Btw, what's your take on this @gsmet?

@gsmet
Copy link
Member

gsmet commented Jul 25, 2022

It has value but I have no idea if our already overloaded CI can support this additional workload. My guess is no.
Also if we start doing this, why only the quickstarts and not the platform, the Quarkiverse... and it's going to be a slippery slope.

I think the problem is more than not many people actually have a regular look at status.quarkus.io. Because if we do, then the current setup is working all right.

It's not as if we had tons of issues.

@rsvoboda
Copy link
Member Author

OK, we do not necessarily need full Quickstarts tests execution. I think having at least the Quickstarts compilation (-DskipTests) step on Java 17 would improve the awareness of QS. The impact on CI would be low.

I think the biggest trouble is that QS has 100 modules from various areas and it's hard to expect that people actively check whether the module related to their area is failing or not.

@rsvoboda
Copy link
Member Author

PR simplified to do just the compilation and not tests execution on QS, using just Java 17

@rsvoboda
Copy link
Member Author

Windows fail https://github.com/quarkusio/quarkus/runs/7519620552?check_suite_focus=true

Error: error: unable to create file independent-projects/tools/base-codestarts/src/main/resources/codestarts/quarkus-extension/code/extension-codestart/java/runtime/src/main/codestarts/quarkus/{extension.id}-codestart/base/src/main/resources/META-INF/resources/index.entry.qute.html: Filename too long

Error: The process 'C:\Program Files\Git\bin\git.exe' failed with exit code 1

@quarkus-bot

This comment has been minimized.

@jmartisk
Copy link
Contributor

@rsvoboda Could you try this as a solution to the windows failure? https://stackoverflow.com/a/22575737/615104

@jmartisk
Copy link
Contributor

jmartisk commented Jul 27, 2022

Ah, actually that's not in the job that you're adding, that's in something that we already have, it might need a separate issue for handling it

UPDATE: #26955 should fix it

@jmartisk
Copy link
Contributor

In any case +1 to this from me, it takes ~10 minutes which is not that bad, and it will notify us about needed QS changes when doing breaking changes.

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 2, 2022

Failing Jobs - Building 9826c31

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/smallrye-reactive-messaging-kafka/deployment 
! Skipped: integration-tests/kafka-oauth-keycloak integration-tests/kafka-sasl-elytron integration-tests/kubernetes/quarkus-standard-way-kafka and 3 more

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3 line 54 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 4 State{lastRun=3, running=true, inProgress=true, run=1, passed=0, failed=1, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
	at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
	at io.quarkus.smallrye.reactivemessaging.kafka.deployment.testing.KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.testContinuousTestingScenario3(KafkaDevServicesContinuousTestingWorkingAppPropsTestCase.java:54)

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: integration-tests/reactive-messaging-amqp 

📦 extensions/smallrye-reactive-messaging-amqp/deployment

io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest.testConsumerUpdate - More details - Source on GitHub

java.util.concurrent.RejectedExecutionException
	at org.jboss.threads.RejectingExecutor.execute(RejectingExecutor.java:38)
	at org.jboss.threads.EnhancedQueueExecutor.rejectShutdown(EnhancedQueueExecutor.java:2076)

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

OK, it's a good trade-off I think.
Let's see if it brings something to the plate!

@gsmet gsmet merged commit 986f3d8 into quarkusio:main Aug 3, 2022
@quarkus-bot quarkus-bot bot added this to the 2.12 - main milestone Aug 3, 2022
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.

None yet

5 participants