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

Jakarta, Spring Boot 3 and Spring 6 #917

Merged
merged 25 commits into from
Feb 3, 2023
Merged

Conversation

MediaMarco
Copy link
Contributor

@MediaMarco MediaMarco commented Jan 11, 2023

This is a pull request for a possible 4.x (?) version which includes the migration to the jakarta namespace. Spring Boot 3 and Spring 6 are working, Java 17 or higher is required.

Currently excluded modules:

Cassandra (Netflix Astyanax is retired, Cassandra Thrift is gone in latest Cassandra version, this module needs some work)
DynamoDB (Uses AWS SDK for Java v1 which only supports Java up to 16, has to be migrated to AWS SDK for Java v2)

… Boot 3 and Spring 6 - Currently missing modules: CDI (some more research/help needed to fix tests), Shiro (still no jakarta version available) and Cassandra (outdated)
@bennetelli
Copy link
Member

Thank you so much, @MediaMarco <3 :)

pom.xml Outdated Show resolved Hide resolved
appengine/pom.xml Outdated Show resolved Hide resolved
cdi/pom.xml Outdated Show resolved Hide resolved
dreis2211 and others added 3 commits January 12, 2023 10:57
* Reduce overhead of TogglzTestExecutionListener

Signed-off-by: Christoph Dreis <christoph.dreis@innogames.com>

* Fix InMemoryStateRepositoryTest

Signed-off-by: Christoph Dreis <christoph.dreis@innogames.com>

Signed-off-by: Christoph Dreis <christoph.dreis@innogames.com>
// @formatter:on
@Bean
public InMemoryUserDetailsManager userDetailsService() {
UserDetails admin = User.withDefaultPasswordEncoder().username("admin").password("pwd").roles("ADMIN", "USER").build();
Copy link

Choose a reason for hiding this comment

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

The method withDefaultPasswordEncoder is marked as deprecated where the explanation says that it's recommended just for demo purposes but not for production. We use it exactly for demo purposes, so my suggestion is to add annotation @SupressWarnings("deprecation") on top of our method userDetailsService

@bennetelli
Copy link
Member

bennetelli commented Jan 13, 2023

@MediaMarco @OleksandrShkurat wow guys! aweseome what's going on here :) thank you so much for your help and your time you invested in moving this forward :)

@codecov-commenter
Copy link

codecov-commenter commented Jan 13, 2023

Codecov Report

Base: 55.35% // Head: 55.90% // Increases project coverage by +0.54% 🎉

Coverage data is based on head (c678eac) compared to base (846c623).
Patch coverage: 56.66% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #917      +/-   ##
============================================
+ Coverage     55.35%   55.90%   +0.54%     
- Complexity      908      913       +5     
============================================
  Files           180      177       -3     
  Lines          4572     4495      -77     
  Branches        594      591       -3     
============================================
- Hits           2531     2513      -18     
+ Misses         1881     1820      -61     
- Partials        160      162       +2     
Impacted Files Coverage Δ
...gine/repository/FixedNamespaceStateRepository.java 100.00% <ø> (ø)
...rc/main/java/org/togglz/cdi/spi/CDIBeanFinder.java 0.00% <0.00%> (ø)
...src/main/java/org/togglz/console/RequestEvent.java 0.00% <ø> (ø)
...in/java/org/togglz/console/RequestHandlerBase.java 3.84% <ø> (ø)
.../java/org/togglz/console/TogglzConsoleServlet.java 0.00% <ø> (ø)
...a/org/togglz/console/handlers/ResourceHandler.java 13.63% <ø> (ø)
.../togglz/console/handlers/edit/EditPageHandler.java 4.76% <ø> (ø)
...ogglz/console/handlers/index/IndexPageHandler.java 7.31% <ø> (ø)
...in/java/org/togglz/console/model/FeatureModel.java 25.71% <ø> (ø)
.../java/org/togglz/console/model/ParameterModel.java 0.00% <ø> (ø)
... and 32 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@OleksandrShkurat
Copy link

@bennetelli thank you. What do you think, is the folder microprofile-config still needed? As I can see, it's an orphaned part of some old removed logic. Am I right?

@MediaMarco
Copy link
Contributor Author

CDI module works again, the failing test was fixed.

@@ -15,7 +15,7 @@ jobs:
strategy:
fail-fast: false
matrix:
jdk: [8, 11]
jdk: [17]
Copy link
Member

Choose a reason for hiding this comment

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

do we actually want to set Java 17 as minimum?

Choose a reason for hiding this comment

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

Good question. Spring boot 3 requires Java 17. However, some other modules could work with older versions

Copy link
Member

Choose a reason for hiding this comment

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

it's actually hard to say if and how many people are using togglz with legacy Java. I have actually no clue about the different use cases of the togglz users.

@MediaMarco uses togglz with Spring Boot. Many others as well. But not sure about the distribution of number of Spring Boot users and number of all togglz users.. :/

I just checked it. Quarkus is also on Java 17. So I think we should just got for it.
Even if I would open a poll or something like that, I would not get enough responses to know if it would be a show stopper to set java 17 as minimum. Let's just do it and see if someone actually needs it to be on a older Java version or not.

@bennetelli
Copy link
Member

@bennetelli thank you. What do you think, is the folder microprofile-config still needed? As I can see, it's an orphaned part of some old removed logic. Am I right?

I would say 90% of the togglz users is using togglz with Spring Boot, but I could also be wrong.
If we could get it up and running again without much effort, then this would be great. If not, we could also consider to remove it as it takes a lot of time to maintain all the extensions.

@OleksandrShkurat
Copy link

@bennetelli thank you. What do you think, is the folder microprofile-config still needed? As I can see, it's an orphaned part of some old removed logic. Am I right?

I would say 90% of the togglz users is using togglz with Spring Boot, but I could also be wrong. If we could get it up and running again without much effort, then this would be great. If not, we could also consider to remove it as it takes a lot of time to maintain all the extensions.

Please correct me if I'm wrong but currently, there is no any reference to the folder microprofile-config from any place in the project. It was referenced between 2017 and 2020 like this:

    <profile>
      <id>java8-features</id>
      <activation>
        <jdk>[1.8,)</jdk>
      </activation>
      <modules>
        <module>microprofile-config</module>
      </modules>
    </profile>

and this commit 48e83bb removed this profile making the folder unused since that time

@OleksandrShkurat
Copy link

OleksandrShkurat commented Jan 18, 2023

Additionally, can we discuss somehow, what is the defintion of done for this activity?
Can we collect all the acceptance criteria for the version 4.0 to be published?
We're talking about the major update and it means that some significant changes are expected.
Except following:

  • moving from javax to jakarta
  • upgrading to JDK17
  • switch to Spring-boot 3
  • Spring Framework 6

we could consider some other changes. For example:

  • decomissioning of some part of code that was marked as deprecated earlier
  • complete migration of internal unittests from Junit4 to Junit5 (dependency on Junit4 could be removed, currently is in-progress on my side)
  • review internal relations among project modules (simplify the dependency tree of the project, I've done it locally)
  • extend .editorconfig with more rules for the code style unifying including import ordering

@MediaMarco
Copy link
Contributor Author

Shiro is back!

@MediaMarco
Copy link
Contributor Author

Proposal: Create a 4.x-branch and merge this pull request into it. The open topics that don't really touch the Jakarta / Spring / JDK 17 migration can then be solved as individual pull requests. Wouldn't this be better?

@bennetelli
Copy link
Member

@bennetelli thank you. What do you think, is the folder microprofile-config still needed? As I can see, it's an orphaned part of some old removed logic. Am I right?

I would say 90% of the togglz users is using togglz with Spring Boot, but I could also be wrong. If we could get it up and running again without much effort, then this would be great. If not, we could also consider to remove it as it takes a lot of time to maintain all the extensions.

Please correct me if I'm wrong but currently, there is no any reference to the folder microprofile-config from any place in the project. It was referenced between 2017 and 2020 like this:

    <profile>
      <id>java8-features</id>
      <activation>
        <jdk>[1.8,)</jdk>
      </activation>
      <modules>
        <module>microprofile-config</module>
      </modules>
    </profile>

and this commit 48e83bb removed this profile making the folder unused since that time

That's true. I started a poll some time ago and asked if someone is still using integration A,B and so on. After getting no responses I just disabled some of them to see if someone opens issues.
Looks like nobody complained about the missing microprofile integration.

@bennetelli
Copy link
Member

Additionally, can we discuss somehow, what is the defintion of done for this activity? Can we collect all the acceptance criteria for the version 4.0 to be published? We're talking about the major update and it means that some significant changes are expected. Except following:

* moving from javax to jakarta

* upgrading to JDK17

* switch to Spring-boot 3

* Spring Framework 6

we could consider some other changes. For example:

* decomissioning of some part of code that was marked as deprecated earlier

* complete migration of internal unittests from Junit4 to Junit5 (dependency on Junit4 could be removed, currently is in-progress on my side)

* review internal relations among project modules (simplify the dependency tree of the project, I've done it locally)

* extend `.editorconfig` with more rules for the code style unifying including import ordering

exactly.

I would also add PRs like this one: #896


Be careful when migrating to JUnit5. As far as I remember correctly, the Spock frameworks still depends on JUnit 4. We should keep this in mind

@MediaMarco MediaMarco requested review from bennetelli and OleksandrShkurat and removed request for OleksandrShkurat and bennetelli January 18, 2023 13:20
@kevinstembridge
Copy link
Contributor

kevinstembridge commented Jan 29, 2023

I'm upgrading a side project to Spring Boot 3 and I'm currently blocked by Togglz being incompatible.
Let me know if I can help with anything here. I'd be happy to try out a 4.0 beta once it is created.

@bennetelli
Copy link
Member

I'm upgrading a side project to Spring Boot 3 and I'm currently blocked by Togglz being incompatible. Let me know if I can help with anything here. I'd be happy to try out a 4.0 beta once it is created.

  • dynamodb needs to be updated
  • cassandra as well

As far as I know

@bennetelli bennetelli merged commit 4f6dd94 into togglz:master Feb 3, 2023
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.

None yet

6 participants