Skip to content

KEYCLOAK-13757 update JDG version to 8.1 - testsuite updates#7774

Merged
pdrozd merged 2 commits intokeycloak:masterfrom
tkyjovsk:KEYCLOAK-13757
Jun 3, 2021
Merged

KEYCLOAK-13757 update JDG version to 8.1 - testsuite updates#7774
pdrozd merged 2 commits intokeycloak:masterfrom
tkyjovsk:KEYCLOAK-13757

Conversation

@tkyjovsk
Copy link
Contributor

@tkyjovsk tkyjovsk commented Feb 11, 2021

No description provided.

@mposolda mposolda self-assigned this Feb 12, 2021
@mposolda mposolda requested review from hmlnarik and pdrozd February 12, 2021 08:07
@mposolda mposolda added area/testsuite Indicates an issue on the Testsuite area status/in-review labels Feb 12, 2021
@mposolda
Copy link
Contributor

mposolda commented Feb 12, 2021

@tkyjovsk Do you please have a link to the pipeline run with cluster and cross-dc tests?

@tkyjovsk tkyjovsk force-pushed the KEYCLOAK-13757 branch 2 times, most recently from 319917c to 7280050 Compare February 23, 2021 19:00
@mposolda
Copy link
Contributor

mposolda commented Mar 2, 2021

@tkyjovsk Thanks for the update! Few notes:

  • It will be nice to update HOW-TO-RUN.md and add some info into the cross-dc section about how to run the cross-dc tests with the new and old versions of the infinispan server. IMO the new infinispan server is more important and hence the instructions for it should be default (added to the top) when the instructions for "legacy" server would be better somewhere at the bottom

  • The GH actions should ideally be updated to use the new infinispan server IMO. The option is to use both new and old, but IMO that will be unecessary overhead and will add more error-proneness. So my vote is to keep just new infinispan in the GH actions

  • I guess pipeline should be updated, so that cross-dc tests use both new and old infinispan server? Also I am missing the successful pipeline run with this PR?

@pdrozd
Copy link
Contributor

pdrozd commented Mar 2, 2021

DG 8.x should be the primary for upstream and also for GH actions. 7.x is just for the current stream. The PR to the pipeline is required.

@pskopek
Copy link
Contributor

pskopek commented Mar 8, 2021 •
edited
Loading

The pipeline MR number 8 has been merged.

@hmlnarik hmlnarik requested a review from vramik April 19, 2021 13:32
vramik
vramik suggested changes Apr 19, 2021
View reviewed changes
Copy link
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

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

Thank you @tkyjovsk for the PR. Overall changes looks good to me.

I found new modules a little bit confusing: cache-server/infinispan/infinispan, cache-server/infinispan/datagrid, cache-server/jboss/infinispan, cache-server/jboss/datagrid .. then I saw CacheStatisticsControllerEnricher.isOldInfinispan() method which bringed more light into it. Do we still need to test infinispan 9 or lower in upstream?

IMO it'd be worth to document it somewhere, probably HOW-TO-RUN.md would be correct place as @mposolda suggested.

Please provide links to pipeline runs.

...rc/main/java/org/keycloak/testsuite/arquillian/containers/InfinispanServerConfiguration.java Outdated
Copy link
Contributor

@vramik vramik Apr 19, 2021

Choose a reason for hiding this comment

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

nit: missing copyright

Copy link
Contributor Author

@tkyjovsk tkyjovsk May 31, 2021

Choose a reason for hiding this comment

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

Addressed since d66706e.

...n/java/org/keycloak/testsuite/arquillian/containers/InfinispanServerDeployableContainer.java Outdated
Copy link
Contributor

@vramik vramik Apr 19, 2021

Choose a reason for hiding this comment

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

nit: missing copyright

Copy link
Contributor Author

@tkyjovsk tkyjovsk May 31, 2021

Choose a reason for hiding this comment

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

Addressed since d66706e.

@tkyjovsk
Copy link
Contributor Author

tkyjovsk commented Apr 21, 2021

@vramik Thanks for the review. I'm working on updating the HOWTO doc. I also want to do some refactoring, esp. around the maven profiles. I plan to update the PR after I manage to run the CI tests.

@tkyjovsk tkyjovsk force-pushed the KEYCLOAK-13757 branch 3 times, most recently from fb0dea8 to 13937b8 Compare May 20, 2021 18:03
@hmlnarik hmlnarik requested review from vramik and removed request for hmlnarik May 30, 2021 20:54
@tkyjovsk
Copy link
Contributor Author

tkyjovsk commented May 31, 2021 •
edited by mposolda
Loading

In revision d66706e I did some refactoring.

  • I renamed cache-server-jboss-infinispan to cache-server-legacy-infinispan (and -datagrid modules respectively).
  • I updated the HOW-TO doc with some additional info.

Known issues:

  • the auth-servers-crossdc-undertow profile currently doesn't work (many tests failing) but this is not a regression against the master branch
  • the cache-auth profile doesn't currently work with the legacy Infinispan/Datagrid modules

I propose to fix these issues separately afterwards.

Note: Tests passed.

@tkyjovsk tkyjovsk mentioned this pull request May 31, 2021
Merged
vramik
vramik previously approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@vramik vramik left a comment

Choose a reason for hiding this comment

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

Thank you @tkyjovsk! The PR LGTM.

mposolda
mposolda requested changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@tkyjovsk PR looks good to me. Thanks! Just one note: Is it possible to update HOW-TO-RUN.md with some notes and TODOs for the configurations and combinations, which doesn't work as you mentioned in the comment above? Ideally if you create JIRA for those issues and mention the JIRA numbers directly in HOW-TO-RUN (So that in the future, the reader of HOW-TO-RUN can evaluate if JIRA was already fixed etc).

This update in HOW-TO-RUN fortunately doesn't need another pipeline run, so hope it is not a big issue :-)

Besides that, I am leaving to @pdrozd approve before merge this PR.

iankko
iankko reviewed Jun 1, 2021
View reviewed changes
testsuite/integration-arquillian/HOW-TO-RUN.md Outdated
Copy link
Contributor

@iankko iankko Jun 1, 2021

Choose a reason for hiding this comment

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

Just nitpick, I would drop the here. IOW so it reads: For Data Grid 7 and older use: ...

Copy link
Contributor Author

@tkyjovsk tkyjovsk Jun 2, 2021

Choose a reason for hiding this comment

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

Addressed in 40d2230.

iankko
iankko previously approved these changes Jun 1, 2021
View reviewed changes
Copy link
Contributor

@iankko iankko left a comment

Choose a reason for hiding this comment

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

LGMT AFAICT (based just on code review, didn't manually test the change, but since the referenced pipeline run returns zero failures, assuming it works OK)

@tkyjovsk tkyjovsk dismissed stale reviews from iankko and vramik via 40d2230 June 2, 2021 23:55
@tkyjovsk
Copy link
Contributor Author

tkyjovsk commented Jun 2, 2021 •
edited
Loading

@mposolda I addressed your concerns in revision 40d2230. Created 2 new JIRAs and mentioned them in the HOW-TO-RUN doc.

https://issues.redhat.com/browse/KEYCLOAK-18335
https://issues.redhat.com/browse/KEYCLOAK-18336

mposolda
mposolda previously approved these changes Jun 3, 2021
View reviewed changes
Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

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

@tkyjovsk Thanks!

@tkyjovsk
Copy link
Contributor Author

tkyjovsk commented Jun 3, 2021

Revision 6c753ce includes commit which is needed after #8106 was merged.

@pdrozd pdrozd merged commit 1033b27 into keycloak:master Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mposolda mposolda mposolda left review comments

+3 more reviewers

@pdrozd pdrozd pdrozd approved these changes

@vramik vramik vramik left review comments

@iankko iankko iankko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@mposolda mposolda

Labels

area/testsuite Indicates an issue on the Testsuite area

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tkyjovsk @mposolda @pdrozd @pskopek @vramik @iankko