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

APP-2813/APP-2812 - BDK2.0 modules setup #166

Merged
merged 5 commits into from
Aug 3, 2020

Conversation

thibauult
Copy link
Member

No description provided.

@thibauult thibauult requested a review from symphony-hong July 30, 2020 09:43
pom.xml Outdated
@@ -13,11 +13,25 @@

<modules>
<module>symphony-bdk-legacy</module>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create another profile for legacy? it might be confusing if legacy is included in 2.0

Copy link
Member Author

Choose a reason for hiding this comment

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

Profile 2.0 is by default not activated since we don't want to start releasing related modules, but we still want to release legacy modules in any case. So yes, for me legacy modules are also part of the 2.0 (like JUnit5/Vintage : https://junit.org/junit5/docs/current/user-guide/#migrating-from-junit4)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, finally I introduced a legacy profile as well 😅 However it is activated by default which is not the case for the 2.0 one

@thibauult thibauult marked this pull request as ready for review July 31, 2020 06:54
@thibauult thibauult requested a review from a team July 31, 2020 06:54
@thibauult thibauult changed the title APP-2873 BDK2.0 project setup APP-2813/APP-2812 BDK2.0 project setup Jul 31, 2020
@thibauult thibauult changed the title APP-2813/APP-2812 BDK2.0 project setup APP-2813/APP-2812 - BDK2.0 modules setup Jul 31, 2020
return true;
}

return arg.trim().isEmpty();
Copy link
Contributor

@symphony-elias symphony-elias Jul 31, 2020

Choose a reason for hiding this comment

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

Could be simplified in :
return arg == null || arg.trim().isEmpty()
But that's a detail

* @param separator The separator
* @return the resulting string
*/
public static String join(String[] array, String separator) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not using String.join(separator, array) ?

* @return a {@link PrivateKey} instance
* @throws GeneralSecurityException On invalid Private Key
*/
public static PrivateKey parseRSAPrivateKey(final String pemPrivateKey) throws GeneralSecurityException {
Copy link
Contributor

Choose a reason for hiding this comment

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

It could improve readability to extract two methods here, one for PKCS#8 format, one for // PKCS#1 format

@symphony-elias symphony-elias self-requested a review July 31, 2020 12:25
symphony-elias
symphony-elias previously approved these changes Jul 31, 2020
Copy link
Contributor

@symphony-elias symphony-elias left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@symphony-youri symphony-youri left a comment

Choose a reason for hiding this comment

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

Mostly open questions for me I don't have a lot of context
But if you other design documents to share I'd be happy to take a look too


The BDK is divided in 3 different layers:
- `core` that contains the minimal set of classes to configure, authenticate and use
the main APIs
Copy link
Contributor

Choose a reason for hiding this comment

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

how will we decide if something is main or advanced?

- `framework` that provides connectors (or starters) for the main Java frameworks
such as [SpringBoot](https://spring.io/projects/spring-boot),
[MicroProfile](https://projects.eclipse.org/projects/technology.microprofile),
[Micronaut](https://micronaut.io/), [Quarkus](https://quarkus.io/) or [Vertx](https://vertx.io/)
Copy link
Contributor

Choose a reason for hiding this comment

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

with JDK project Loom, Vertx is dead! :P

* Interface used to perform HTTP requests performed by the generated Swagger code.
*/
@API(status = API.Status.STABLE)
public interface ApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

did we consider other http clients? (like feign for instance?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the one that we will implement soon is the WebClient for Spring integration

Copy link
Contributor

Choose a reason for hiding this comment

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

That one because it is a reactive one has a terrible API (IMHO) but I hope we will hide the complexity of with our own API

final AuthenticateRequest request = new AuthenticateRequest();
request.setToken(generateJwt());

final String sessionToken = loginAuthApi.pubkeyAuthenticatePost(request).getToken();
Copy link
Contributor

Choose a reason for hiding this comment

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

at the Java API level do we need the HTTP method to appear (post)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this class is just a temporary hello world, we will hide those low-level clients in cleaner services


<dependency>
<groupId>ch.qos.logback</groupId>
<artifactId>logback-classic</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

In the Spring Boot starter log4j is used, do you know if there is standard one to prefer for Symphony projects?

Copy link
Member Author

Choose a reason for hiding this comment

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

For tests and hello worlds samples I keep using logback, as this dependency is not directly provided by the SDK library (only slf4j is)


private static final String POD_BASE_URL = "https://devx1.symphony.com";

public static void main(String[] args) throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

in the SDK do we provide an Java API close to the REST API or do we provide a Java API that hides a lot of the complexity?
with the second version for instance I would expect to be able to initialize the SDK with auth info and then to perform queries without passing any auth tokens, all of that being taken care internally

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes of course the SDK will provide services that wrap this complexity. We just did not started building them yet :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I created this sample class mainly to confirm that the code generation was working well

@thibauult thibauult merged commit 2696422 into finos:master Aug 3, 2020
@thibauult thibauult deleted the arch/setup_bdk2_modules branch August 3, 2020 08:11
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.

4 participants