Skip to content

Adds recipe UpdateSmartLifecycleDefaultPhase and FlywayMigration #324

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

Closed
wants to merge 1 commit into from

Conversation

kunli2
Copy link
Contributor

@kunli2 kunli2 commented Mar 28, 2023

WIP (please review testing first only)

This PR is part of the migration of Spring Boot 3.0 #257.

Add tests per my understanding of the requirement.

  1. For recipe UpdateSmartLifecycleDefaultPhase, the requirement is Update phases for graceful shutdown, it just replaces Integer.MAX_VALUE with SmartLifecycle.DEFAULT_PHASE in method SmartLifecycle getPhase() .
    Need some Spring boot expertise to confirm my understanding of the requirement is correct.
  2. For recipe FlywayMigration.
    I am not sure about the requirement for now. just added some code snippets in the test by following the description Migrate to Flyway 9.0.

@kunli2 kunli2 requested a review from nmck257 March 28, 2023 16:51
@kunli2
Copy link
Contributor Author

kunli2 commented Mar 28, 2023

Hi @nmck257, I don't have a lot of experience with Spring boot, and we need some help from Spring boot experts to confirm the understanding of the requirement is correct.
By any chance, if you happen to know this specific knowledge, Could you help take a look and help us to confirm the exact requirement here? appreciate for it.

@nmck257
Copy link
Collaborator

nmck257 commented Mar 28, 2023

acknowledged -- should be able to review tomorrow

@nmck257 nmck257 self-assigned this Mar 28, 2023
@nmck257
Copy link
Collaborator

nmck257 commented Mar 29, 2023

For UpdateSmartLifecycleDefaultPhase:

However:

  • the language of 31714 above implies that there was no (reliable) way to have custom SmartLifecycle behaviors interweave with graceful shutdown before this change, and I tend to agree. So, I'm not sure that there's a canonical or even common "before" use case which we could write a recipe to migrate
  • your draft recipe replaces usages of Integer.MAX_VALUE in a getPhase method with the graceful shutdown constant; something like that might be useful, but,
    • I would consider that a 2.5 migration
    • the recipe would need to be able to infer whether the project code is attempting to interact with graceful shutdown's lifecycle, or, if it's using Integer.MAX_VALUE because it wants to do something unrelated to web graceful shutdown, very late in the shutdown process. I'm not sure that's feasible

So overall, I'd probably put that use case on the shelf.

For the Flyway piece, I haven't used Flyway and I'm less confident, but it sounds like the effect of this change is that the Callback and JavaMigration declarations might be "registered" in a different order than before, and that may or may not change the overall behavior of the Flyway migrations, which may or may not impact the app's overall correctness.

If we could precisely identify scenarios where that would be a problem, then maybe a recipe could help "correct" the ordering. But I'm not sure we can identify those cases, and I think that trying to "correct the ordering" globally would probably be over-invasive. So, I'd probably shelve this use case too.

@knutwannheden
Copy link
Contributor

@kunli2 The CI build currently complains because the license header is missing in all the new files.

@kunli2
Copy link
Contributor Author

kunli2 commented Mar 29, 2023

Thanks @nmck257 for looking into this and for so much helpful feedback!

@kunli2 kunli2 mentioned this pull request Apr 4, 2023
14 tasks
@kunli2 kunli2 closed this Apr 4, 2023
@timtebeek timtebeek deleted the kunli/smartlife-cycle branch July 18, 2023 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants