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

Disallow public final methods on recorders #21080

Merged
merged 1 commit into from
Oct 31, 2021

Conversation

stuartwdouglas
Copy link
Member

These can cause problems as the results silently won't be recorded.

This is potentially a breaking change, although I don't think it should
cause issues in practice, as there is no real use case for final public
recorder methods.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 29, 2021

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

Failing Jobs - Building e897aec

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build Failures Logs Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: devtools/cli integration-tests/native-config-profile 

📦 devtools/cli

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-cli: Failed to build quarkus application

📦 integration-tests/native-config-profile

Failed to execute goal io.quarkus:quarkus-maven-plugin:999-SNAPSHOT:build (default) on project quarkus-integration-test-native-config-profile: Failed to build quarkus application

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.

Tested it on the current version of the PanacheKotlinHibernateOrmRecorder and it triggers the exception. With this we would no longuer have silent bugs like in #20882

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 29, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 29, 2021

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

Failing Jobs - Building 931df34

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 Build Failures Logs Raw logs
Native Tests - Data3 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/hibernate-orm/runtime 
! Skipped: docs extensions/hibernate-envers/deployment extensions/hibernate-envers/runtime and 101 more

📦 extensions/hibernate-orm/runtime

Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (enforce) on project quarkus-hibernate-orm: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.


⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/hibernate-orm/runtime 
! Skipped: docs extensions/hibernate-envers/deployment extensions/hibernate-envers/runtime and 101 more

📦 extensions/hibernate-orm/runtime

Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (enforce) on project quarkus-hibernate-orm: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.


⚙️ JVM Tests - JDK 17 #

- Failing: extensions/hibernate-orm/runtime 
! Skipped: docs extensions/hibernate-envers/deployment extensions/hibernate-envers/runtime and 101 more

📦 extensions/hibernate-orm/runtime

Failed to execute goal org.apache.maven.plugins:maven-enforcer-plugin:3.0.0-M3:enforce (enforce) on project quarkus-hibernate-orm: Some Enforcer rules have failed. Look above for specific messages explaining why the rule failed.


⚙️ Native Tests - Data3 #

- Failing: integration-tests/hibernate-orm-panache-kotlin 

📦 integration-tests/hibernate-orm-panache-kotlin

io.quarkus.it.panache.TestEndpointRunner.testModel - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.hibernate.orm.panache.kotlin.deployment.KotlinPanacheResourceProcessor#build threw an exception: java.lang.RuntimeException: Public method public final void io.quarkus.hibernate.orm.panache.kotlin.runtime.PanacheKotlinHibernateOrmRecorder.setEntityToPersistenceUnit(java.util.Map) cannot be proxied as it is final

@geoand
Copy link
Contributor

geoand commented Oct 29, 2021

Needs a rebase now that #20882 is in

@famod
Copy link
Member

famod commented Oct 29, 2021

Needs a rebase after #21092 went in.

These can cause problems as the results silently won't be recorded.

This is potentially a breaking change, although I don't think it should
cause issues in practice, as there is no real use case for final public
recorder methods.
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 30, 2021

Failing Jobs - Building 6c70479

Status Name Step Failures 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: integration-tests/vault-agroal 

📦 integration-tests/vault-agroal

io.quarkus.vault.AgroalVaultITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

io.quarkus.vault.AgroalVaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

io.quarkus.vault.VaultKv1ITCase. - More details - Source on GitHub

java.lang.RuntimeException: Error waiting for test resource future to finish.
	at io.quarkus.test.common.TestResourceManager.waitForAllFutures(TestResourceManager.java:151)
	at io.quarkus.test.common.TestResourceManager.start(TestResourceManager.java:136)

@stuartwdouglas stuartwdouglas merged commit 9aaa192 into quarkusio:main Oct 31, 2021
@quarkus-bot quarkus-bot bot added this to the 2.5 - main milestone Oct 31, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Oct 31, 2021
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.

4 participants