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

[WFLY-12342] Add server probes for readiness health check #223

Merged

Conversation

mchoma
Copy link

@mchoma mchoma commented Sep 11, 2020

[DO NOT MERGE] This needs WF21 changes in to be valid.

Changes relater to WFLY-12342:

  • adjust existing tests to work with new changes
  • added new tests for new fetures in WFLY-12342

CI run: ✔️ https://eap-qe-jenkins.rhev-ci-vms.eng.rdu2.redhat.com/job/eap-7.x-microprofile-custom-build-from-sources-face/120/
(metrics tests are expected to fail currently with 21 and are tracked separately by https://issues.redhat.com/browse/WFLY-13779)

Please make sure your PR meets the following requirements:

  • Pull Request contains a description of the changes
  • Pull Request does not include fixes for multiple issues/topics
  • Code is formatted, imports ordered, code compiles and tests are passing
  • Link to the passing job is provided
  • Code is self-descriptive and/or documented
  • Description of the tests scenarios is included (see Update PR template to include TPG stuff #46)

@mchoma mchoma force-pushed the WFLY-12342_readiness_health_check branch 2 times, most recently from 9b232e2 to 7a81ca3 Compare September 15, 2020 07:52
@mchoma mchoma requested a review from istraka September 15, 2020 08:05
@mchoma
Copy link
Author

mchoma commented Sep 15, 2020

I would appreciate if someone with more experience in this testsuite area will have a look. @istraka you was suggested by github, is there chance you can have a look? :)

@mchoma mchoma marked this pull request as draft September 15, 2020 08:06
@mchoma mchoma marked this pull request as ready for review September 15, 2020 08:06
@mchoma mchoma marked this pull request as draft September 15, 2020 08:07
Copy link
Member

@istraka istraka left a comment

Choose a reason for hiding this comment

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

One unnecessary change.
One open suggestion?

Otherwise LGTM.

@mchoma mchoma force-pushed the WFLY-12342_readiness_health_check branch from 7a81ca3 to d5393bd Compare September 24, 2020 13:57
Changes relater to WFLY-12342:
- adjust existing tests to work with new changes
- added new tests for new fetures in WFLY-12342
@mchoma mchoma force-pushed the WFLY-12342_readiness_health_check branch from d5393bd to 02813e0 Compare September 24, 2020 14:30
@mchoma
Copy link
Author

mchoma commented Sep 24, 2020

Question is when to merge it? Wildfly 21.0.Beta1 is released, so there is already publicly available distro this testsuite will work with.

But that can potentially break running 7.3.x releases. For EAP XP1 there is branch XP1. Have you already have such situation in past? Testing 7.3.x could potentially use XP1 branch as well not to have a lot of branches.

@istraka
Copy link
Member

istraka commented Sep 24, 2020

@mchoma I have been out of this project for a while so I do not know. IMO if 7.3.z used XP1 branch it could create a confusion. What about creating 7.3.z branch? WDYT @fabiobrz @honza-kasik?

@mchoma
Copy link
Author

mchoma commented Sep 24, 2020

I agree it is confusing. But XP1 and 7.3.x have common Observability specs so I would say it is correct from feature perspective. We can rename it to something what fits both. My motivation is to reduce number of necessary branches to avoid cherry-picking. But of course branch per stream is most correct. But when you will have fix in observability specs tests you will need to make sure they are in all 3 branches master, 7.3.x and XP1. Thats tradeoff.

Also this make me think we will need XP2 branch and run XP2 branch when processing XP2 build.

@mchoma
Copy link
Author

mchoma commented Oct 6, 2020

Actually MP health changes will be present in EAP 7.3.4 Base as well. That mean testsuite master branch will be still valid for testing EAP 7.3.4 Base.

So once EAP 7.3.3 Base will be tested we can merge this. We will need to cherry pick to XP1 branch as well.

@mchoma
Copy link
Author

mchoma commented Oct 15, 2020

I believe as EAP 7.3.3 Base was released this is safe to merge now into master.

@mchoma mchoma marked this pull request as ready for review October 15, 2020 12:59
@mchoma mchoma changed the title [WIP] [WFLY-12342] Add server probes for readiness health check WFLY-12342] Add server probes for readiness health check Oct 15, 2020
@mchoma mchoma changed the title WFLY-12342] Add server probes for readiness health check [WFLY-12342] Add server probes for readiness health check Oct 15, 2020
@mnovak1
Copy link
Member

mnovak1 commented Oct 20, 2020

I think this is ok to merge as master should work with latest WF.

I guess now we're facing question whether to use branches for EAP 7.3.z/XP1,2,3 or have less branches and use profiles to run separate groups of tests. For example XP 3 will contain MP 4.0 where might be other incompatible changes so tests for those should not be executed with XP1,2 and so on.
I don't have strong preference here. I'm used to profiles as cherry-picking to different branches makes it harder track what is where.

Edit: Now i'm thinking that there are MP specs directly included in modules thus to avoid version conflicts we would need to split those modules per MP spec version as it's done in https://github.com/rhoar-qe/thorntail-test-suite/tree/master/microprofile

@mchoma mchoma assigned mnovak1 and unassigned honza-kasik Oct 20, 2020
@mchoma
Copy link
Author

mchoma commented Oct 20, 2020

@mnovak1 Thank you for review. Can you merge it please, then?

I have put on meeting agenda how we should manage XP3/ MP4. If TT comes to best is module/per spec we should probably copy that approach. We should double check if they are satisfied with this model. I see some drawback as well.

@fabiobrz
Copy link
Member

I think this is ok to merge as master should work with latest WF.

I guess now we're facing question whether to use branches for EAP 7.3.z/XP1,2,3 or have less branches and use profiles to run separate groups of tests. For example XP 3 will contain MP 4.0 where might be other incompatible changes so tests for those should not be executed with XP1,2 and so on.
I don't have strong preference here. I'm used to profiles as cherry-picking to different branches makes it harder track what is where.

Edit: Now i'm thinking that there are MP specs directly included in modules thus to avoid version conflicts we would need to split those modules per MP spec version as it's done in https://github.com/rhoar-qe/thorntail-test-suite/tree/master/microprofile

Thanks @mnovak1 - so, following up suggestions from you and Martin, IIUC we should implement packages vs. spec version here, right? Would this concretely mean a major refactoring of the TS is needed?

@mnovak1
Copy link
Member

mnovak1 commented Oct 20, 2020

@fabiobrz @mchoma I've found myself to create issue for this a half year ago :-) - #215. I've added there suggestion with profiles. Feel free to edit or comment it so we can reach some consensus.

@mchoma
Copy link
Author

mchoma commented Oct 20, 2020

@mnovak1 Can you merge as we agree this is safe to be merged into master now. We are waiting for this changes to be present for XP2 pipeline.

I found discussion about organisation of testsuite unrelated. But for sure we need to continue with its discussion on issue you linked. Changes of this MR are not related to MP spec upgrade. We just added vendor specific probes to existing MP Health Spec.

@mnovak1 mnovak1 merged commit 9efd68f into jboss-eap-qe:master Oct 20, 2020
@mnovak1
Copy link
Member

mnovak1 commented Oct 20, 2020

Done.

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.

5 participants