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

feat: upgrade to WireMock 3 #35

Merged
merged 1 commit into from
Oct 17, 2023
Merged

Conversation

wjglerum
Copy link
Contributor

@wjglerum wjglerum commented Sep 8, 2023

Upgrades Wiremock to the new v3 with updated maven coordinates. See https://github.com/wiremock/wiremock/releases

And enables the global response templating with a config key.

@chberger chberger self-requested a review September 8, 2023 07:28
Copy link
Contributor

@chberger chberger left a comment

Choose a reason for hiding this comment

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

Many thanks for this PR. Overall, it looks very good. It would be cool, if you could add a test that verifies the basic templating feature once it is enabled.

@chberger
Copy link
Contributor

chberger commented Sep 8, 2023

@all-contributors add @wjglerum for code

@allcontributors
Copy link
Contributor

@chberger

I've put up a pull request to add @wjglerum! 🎉

@wjglerum
Copy link
Contributor Author

wjglerum commented Sep 8, 2023

Many thanks for this PR. Overall, it looks very good. It would be cool, if you could add a test that verifies the basic templating feature once it is enabled.

@chberger I was thinking about that too. But I'm not to sure on where we should add those tests in this project. I see some test code, but there isn't a lot.

Any ideas?

@chberger
Copy link
Contributor

chberger commented Sep 8, 2023

Many thanks for this PR. Overall, it looks very good. It would be cool, if you could add a test that verifies the basic templating feature once it is enabled.

@chberger I was thinking about that too. But I'm not to sure on where we should add those tests in this project. I see some test code, but there isn't a lot.

Any ideas?

Yeah, the test setup isn't that great. Let me check the code to see where to put it. 👍

@wjglerum
Copy link
Contributor Author

@chberger I added some tests, let me know what you think about this approach

@chberger
Copy link
Contributor

chberger commented Sep 12, 2023

@wjglerum First of all, thank you very much for your contribution. To be honest, I'm currently refactoring the whole repo, because a lot of stuff isn't working (e.g. as you already figured out with integration tests). So I would need a little bit more time before I will merge your changes.

Basically, the idea is to provide a 0.X version of quarkiverse-wiremock with Wiremock 2.35.x with Quarkus 2.16.X.Finalsupport and a 1.X version that supports Wiremock 3.X and Quarkus 3.X. LTS. However, I'd like to keep the 0.X version backward compatible, but I will change the API starting from 1.X. I'm almost finished with my re-factoring so it would be nice if you could wait a little bit more, otherwise we have conflicting changes. Once my refactoring is finished, you can rebase your changes onto main.

Anyway, a short feedback to your PR:
I would also place those templating tests in deployment. However, I would use QuarkusDevModeTests because you can then test the templating feature with a hot-reload. You'll find an example in my refactoring soon. In addition, the test module (IMHO bad name) exposes an annotation to inject the running wiremock instance, so you can omit those beforeAll callbacks.

@chberger
Copy link
Contributor

chberger commented Sep 12, 2023

Furthermore, I'd prefer functional PRs. The current PR includes at least three topics (integration tests, WireMock 3-upgrade, global templating). I know it's also because of the current repo condition, but as I said: Please give me little bit more time, then u are more than welcome for any contribution. :-) But currently I see the risk that you do stuff which I have already done or which might be obsolete.

@wjglerum
Copy link
Contributor Author

Thanks for your explanation @chberger! And good to hear you are taking care of this project 👍

Fully agree on the functional PRs, like you mentioned it's more the current state that made me do multiple changes in one.

I'll give you some time to do the refactoring and rebase on top of your work later 👍

@wjglerum
Copy link
Contributor Author

hi @chberger how is the refactoring going? Need any help? Or can I check something out already?

@chberger
Copy link
Contributor

hi @wjglerum: Actually I can only work on this in my spare time, thus the refactoring is still ongoing. There is one last problem with the integration-tests and they way how we inject a reference to the mock server. Maybe you're interested in a code review. So I could push my code changes in the next couple of days?

@chberger
Copy link
Contributor

And for sure, the documentation with AsciiDoc and Antora is also still pending ...

@wjglerum
Copy link
Contributor Author

hi @wjglerum: Actually I can only work on this in my spare time, thus the refactoring is still ongoing. There is one last problem with the integration-tests and they way how we inject a reference to the mock server. Maybe you're interested in a code review. So I could push my code changes in the next couple of days?

makes total sense, no rush! For sure, if you open a (draft) PR I can have a look and maybe help fix the integration tests

@chberger
Copy link
Contributor

@wjglerum : Here we go: #40

Any feedback would be highly appreciated.

@chberger
Copy link
Contributor

@wjglerum All right, I guess we're good to go with this PR. But no rush ..

@wjglerum
Copy link
Contributor Author

@wjglerum All right, I guess we're good to go with this PR. But no rush ..

Cool, I'll probably have another look at this tonight. And I'll create another one to target the 0.x.x branch to also enable global response templating there, I have an actual use case for it too :)

@wjglerum wjglerum changed the title Upgrade to Wiremock 3.0.3 feat: upgrade to WireMock 3 Oct 17, 2023
@wjglerum
Copy link
Contributor Author

wjglerum commented Oct 17, 2023

Simplified this PR to just upgrading to WireMock 3, will do a seperate one to enable the response templating too.

@chberger chberger self-requested a review October 17, 2023 20:08
@chberger chberger merged commit 67fbd1f into quarkiverse:main Oct 17, 2023
@wjglerum wjglerum deleted the wiremock3 branch October 18, 2023 06:15
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.

2 participants