You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@fineract.apache.org by GitBox <gi...@apache.org> on 2021/11/21 13:41:11 UTC
[GitHub] [fineract] ptuomola opened a new pull request #1975: Upgrade to Spring Boot 2.6.0
ptuomola opened a new pull request #1975:
URL: https://github.com/apache/fineract/pull/1975
## Description
Describe the changes made and why they were made.
Ignore if these details are present on the associated [Apache Fineract JIRA ticket](https://github.com/apache/fineract/pull/1284).
## Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
- [ ] Write the commit message as per https://github.com/apache/fineract/#pull-requests
- [ ] Acknowledge that we will not review PRs that are not passing the build _("green")_ - it is your responsibility to get a proposed PR to pass the build, not primarily the project's maintainers.
- [ ] Create/update unit or integration tests for verifying the changes made.
- [ ] Follow coding conventions at https://cwiki.apache.org/confluence/display/FINERACT/Coding+Conventions.
- [ ] Add required Swagger annotation and update API documentation at fineract-provider/src/main/resources/static/api-docs/apiLive.htm with details of any API changes
- [ ] Submission is not a "code dump". (Large changes can be made "in repository" via a branch. Ask on the developer mailing list for guidance, if required.)
FYI our guidelines for code reviews are at https://cwiki.apache.org/confluence/display/FINERACT/Code+Review+Guide.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#issuecomment-977842201
> Overall looks good to me. Just one caveat: at the moment there is no compatible Spring Cloud release for Spring Boot 2.6.0. Just in case someone is using that combination in customized projects. For the Fineract release it's irrelevant.
>
> @ptuomola any ideas why the Travis stuff is failing? Other than that I think this can be approved.
The GitHub Gradle build runs exactly the same code as the Travis one, so I think we're safe here. Overall I think the Travis is much more susceptible to all kinds of disturbances (load, timing etc) than the GitHub one - and the Github is faster. I think we should reduce the Travis one to just run the SonarQube analysis (as that can't be integrated to GitHub).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
ptuomola commented on pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#issuecomment-977764933
@vidakovic This upgrades the Spring Boot to 2.6.0 - an upgrade that was unblocked by the config file rework. If you can approve and merge this, as next step I'll then rework the OAuth and twofactor authentication, and write some test cases for both of them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola closed pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
ptuomola closed pull request #1975:
URL: https://github.com/apache/fineract/pull/1975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on a change in pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
vidakovic commented on a change in pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#discussion_r756009170
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareBasicAuthenticationFilter.java
##########
@@ -59,33 +58,34 @@
*
* If multi-tenant and basic auth credentials are invalid, a http error response is returned.
*/
-@Service
+
@Profile("basicauth")
public class TenantAwareBasicAuthenticationFilter extends BasicAuthenticationFilter {
private static boolean firstRequestProcessed = false;
private static final Logger LOG = LoggerFactory.getLogger(TenantAwareBasicAuthenticationFilter.class);
- private final BasicAuthTenantDetailsService basicAuthTenantDetailsService;
- private final ToApiJsonSerializer<PlatformRequestLog> toApiJsonSerializer;
- private final ConfigurationDomainService configurationDomainService;
- private final CacheWritePlatformService cacheWritePlatformService;
- private final NotificationReadPlatformService notificationReadPlatformService;
+ @Autowired
Review comment:
A compromise here could be to use Lombok with "RequiredArgsConstructor"... then we can keep the same dependency injection strategy without being annoying (aka having to write all the constructor boilerplate code). Personally the "@Autowired" are OK for me... not sure if others have an opinion here.
If we introduce Lombok (I really would like) then I prefer the combination of "@RequiredArgsConstructor" and final class attributes.
My 2 cents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic merged pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
vidakovic merged pull request #1975:
URL: https://github.com/apache/fineract/pull/1975
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic commented on pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
vidakovic commented on pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#issuecomment-977826524
Overall looks good to me. Just one caveat: at the moment there is no compatible Spring Cloud release for Spring Boot 2.6.0. Just in case someone is using that combination in customized projects. For the Fineract release it's irrelevant.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] vidakovic edited a comment on pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
vidakovic edited a comment on pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#issuecomment-977826524
Overall looks good to me. Just one caveat: at the moment there is no compatible Spring Cloud release for Spring Boot 2.6.0. Just in case someone is using that combination in customized projects. For the Fineract release it's irrelevant.
@ptuomola any ideas why the Travis stuff is failing? Other than that I think this can be approved.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [fineract] ptuomola commented on a change in pull request #1975: Upgrade to Spring Boot 2.6.0
Posted by GitBox <gi...@apache.org>.
ptuomola commented on a change in pull request #1975:
URL: https://github.com/apache/fineract/pull/1975#discussion_r756027492
##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/filter/TenantAwareBasicAuthenticationFilter.java
##########
@@ -59,33 +58,34 @@
*
* If multi-tenant and basic auth credentials are invalid, a http error response is returned.
*/
-@Service
+
@Profile("basicauth")
public class TenantAwareBasicAuthenticationFilter extends BasicAuthenticationFilter {
private static boolean firstRequestProcessed = false;
private static final Logger LOG = LoggerFactory.getLogger(TenantAwareBasicAuthenticationFilter.class);
- private final BasicAuthTenantDetailsService basicAuthTenantDetailsService;
- private final ToApiJsonSerializer<PlatformRequestLog> toApiJsonSerializer;
- private final ConfigurationDomainService configurationDomainService;
- private final CacheWritePlatformService cacheWritePlatformService;
- private final NotificationReadPlatformService notificationReadPlatformService;
+ @Autowired
Review comment:
I think this is a case of personal preferences... personally I like declaring the variables as @Autowired more than the "huge constructor with lots of parameters" approach, as it's simply less code: you just need to declare the variables, not repeat them three times (one declaration and twice in constructor). But you're right that it differs from how most of the classes are currently written...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@fineract.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org