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

Polish recipe for migration JakartaEE Authz/Authn #167

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

mabartos
Copy link
Contributor

Fixes #166

@tkvangorder Hello, could you please check it? Thank you :))

More info in the attached issue.

@timtebeek timtebeek added the enhancement New feature or request label Jan 16, 2023
@timtebeek
Copy link
Contributor

Thanks for providing a potential fix to your own issue @mabartos ! I'll have to dive in a bit before I can correctly review this; one thing that surprised me is that there are not associated test changes needed here. Were there no such tests already?

I would expect a test containing both the dependency and old code, and then assert the new dependency and new code.

It's of course not required of you to add this, but it would be helpful I think to guard that this works and keeps working in the future.

@mabartos
Copy link
Contributor Author

mabartos commented Jan 16, 2023

@timtebeek Thanks for your response!

I wasn't sure if it was necessary also to update test cases for each particular recipe. I thought the present test cases are more "type-based" centric, such as some general ones for org.openrewrite.maven.AddDependency, etc.

However, yep, I'll try to make a test case for the Jakarta Authz recipe.

@timtebeek
Copy link
Contributor

Thanks for the quick response and offer to write such a test; let me know if you need any help!

@sambsnyd
Copy link
Member

sambsnyd commented Jan 17, 2023

Do we need both? Perhaps an UpgradeDependency for the earlier version of jakarta and an AddDependency for javax

@sambsnyd sambsnyd merged commit 7176632 into openrewrite:main Jan 19, 2023
@sambsnyd
Copy link
Member

Thanks for the contribution @mabartos !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Polish recipe for migration JakartaEE Authz/Authn
3 participants