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

Add Migration module with OpenRewrite recipes for AxonFramework 4.7 #2597

Merged
merged 11 commits into from
Feb 14, 2023
Merged

Add Migration module with OpenRewrite recipes for AxonFramework 4.7 #2597

merged 11 commits into from
Feb 14, 2023

Conversation

timtebeek
Copy link
Contributor

Fixes #2596. Rough draft for now, to show you the outline of how I would approach this; edits welcome by maintainers. (@gklijs)

Migration guide mistakenly uses axon-config-jakarta, which does not exist.
@timtebeek
Copy link
Contributor Author

timtebeek commented Feb 10, 2023

Tests were failing before because the migration guide mistakenly mentions axon-config-jakarta, whereas it should be axon-configuration-jakarta. Another reason to automate.. @smcvb
https://docs.axoniq.io/reference-guide/axon-framework/upgrading-to-4-7#steps-to-upgrade-from-jakarta-to-jakarta

@timtebeek timtebeek marked this pull request as ready for review February 10, 2023 10:39
@smcvb
Copy link
Member

smcvb commented Feb 10, 2023

Tests were failing before because the migration guide mistakenly mentions axon-config-jakarta, whereas it should be axon-configuration-jakarta. Another reason to automate.. @smcvb https://docs.axoniq.io/reference-guide/axon-framework/upgrading-to-4-7#steps-to-upgrade-from-jakarta-to-jakarta

Thanks for the concern, @timtebeek.
Know that tooling like this is not "not" in place for a lack of desire but rather a matter of focus.
If our resources would allow it, this would've been in place already.

All the better that you're providing a pull request for the matter.
It is an open-source project, after all.

So, thanks for doing this, @timtebeek :-)

@smcvb smcvb requested review from a team, gklijs, CodeDrivenMitch and smcvb and removed request for a team February 10, 2023 11:03
@smcvb smcvb added this to the Release 4.7.2 milestone Feb 10, 2023
@smcvb smcvb added Type: Feature Use to signal an issue is completely new to the project. Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: In Progress Use to signal this issue is actively worked on. labels Feb 10, 2023
@timtebeek
Copy link
Contributor Author

You're welcome; just happy to help, and with the initial module in place it's not that hard to maintain I would say.

No idea how to resolve the final check failure; will you take it from here?

Error: Input required and not supplied: github-token

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

First and foremost, thanks for the effort you've put in here, @timtebeek!

I've got quite a bunch of comments. Some of those details of the migration, but also several around how this works.

Quick apply during lunch.

Co-authored-by: Steven van Beelen <[email protected]>
@timtebeek
Copy link
Contributor Author

Applied the first round of suggestion and answered a couple of questions, any other comments will have to wait until after work. You're welcome to push changes too, just so you know! :)

Copy link
Member

@smcvb smcvb left a comment

Choose a reason for hiding this comment

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

My concerns have been addressed. Hence I'm approving this pull request.
There's still some test work left I assume, but I don't feel like blocking this PR for that.

@gklijs, what's your take on this PR?

Copy link
Contributor

@gklijs gklijs left a comment

Choose a reason for hiding this comment

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

Looks good and complete to me. Just not sure about using the snapshot versions in the readme.

@smcvb smcvb merged commit d39aa84 into AxonFramework:axon-4.7.x Feb 14, 2023
@timtebeek timtebeek deleted the migration-recipes branch February 14, 2023 11:11
@smcvb smcvb added Status: Resolved Use to signal that work on this issue is done. and removed Status: In Progress Use to signal this issue is actively worked on. labels Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority 3: Could Low priority. Issues that are nice to have but have a straightforward workaround. Status: Resolved Use to signal that work on this issue is done. Type: Feature Use to signal an issue is completely new to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants