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

Upgrading to Wicket 10, Spring 6 and related Jakarta EE specs #741

Merged
merged 5 commits into from
Jan 6, 2023
Merged

Upgrading to Wicket 10, Spring 6 and related Jakarta EE specs #741

merged 5 commits into from
Jan 6, 2023

Conversation

ilgrosso
Copy link
Contributor

@ilgrosso ilgrosso commented Jan 5, 2023

Following #740
Before merging this request to master branch, the master branch shall be copied as the new wicket-9.x branch.

Still draft because:

  1. mvn clean install -DskipTests works fine, but test results were not checked yet
  2. replacement for Wicket class removals (in particular ModalWindow) are to be checked

@martin-g
Copy link
Member

martin-g commented Jan 5, 2023

I will create branch wicket-9.x from master and leave master for 10.x

@martin-g
Copy link
Member

martin-g commented Jan 5, 2023

Done: https://github.com/wicketstuff/core/tree/wicket-9.x

martin-g and others added 2 commits January 5, 2023 14:23
Signed-off-by: Martin Tzvetanov Grigorov <mgrigorov@apache.org>
@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 5, 2023

@martin-g
Copy link
Member

martin-g commented Jan 5, 2023

@martin-g the branch should be changed in https://github.com/wicketstuff/core/blob/wicket-9.x/.github/workflows/ci.yml

Fixed it! Thanks!

@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 5, 2023

All tests are passing locally but GitHub Actions goes error

Error:  org.wicketstuff.pageserializer.kryo2.KryoSerializerTest.homepageRendersSuccessfully  Time elapsed: 0.114 s  <<< FAILURE!
org.opentest4j.AssertionFailedError: The produced data length is not correct! ==> expected: <777> but was: <776>

See https://github.com/wicketstuff/core/actions/runs/3847139651/jobs/6553240311#step:5:10121

@martin-g
Copy link
Member

martin-g commented Jan 5, 2023

This is a known issue.
I'd just remove this assertion. It is too unstable.

@ilgrosso ilgrosso marked this pull request as ready for review January 5, 2023 14:26
@ilgrosso ilgrosso requested a review from dashorst January 5, 2023 14:26
@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 5, 2023

@martin-g now that build is green, I'd ask you to review this PR, especially for the changes due to Wicket upgrade.

In particular ModalWindow -> ModalDialog since it seems that the latter has way less methods than the former, and I've simply removed the calls to missing methods: please advice if there is a better way, thanks.

Copy link
Member

@martin-g martin-g left a comment

Choose a reason for hiding this comment

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

LGTM!

About the ModalWindow->ModalDialog changes - we can always fix them later if there are issues!

@ilgrosso ilgrosso merged commit a0805d0 into wicketstuff:master Jan 6, 2023
@ilgrosso ilgrosso deleted the WICKET_10x branch January 6, 2023 11:19
@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 6, 2023

Thank you @martin-g !

@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 6, 2023

It looks there is some issue with OSS SNAPSHOT deployment about credentials

@martin-g
Copy link
Member

martin-g commented Jan 6, 2023

I'm uploading -SNAPSHOT from my machine...

@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 6, 2023

Ah, thank you

@martin-g
Copy link
Member

martin-g commented Jan 6, 2023

Forgot to mention that the upload/deploy is done!

@solomax
Copy link
Contributor

solomax commented Jan 9, 2023

great job @ilgrosso!
Thanks a lot :)))

@ilgrosso
Copy link
Contributor Author

ilgrosso commented Jan 9, 2023

@solomax my pleasure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants