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

40 : Added keycloak configuration #1

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

rehammuzzamil
Copy link

@rehammuzzamil rehammuzzamil commented May 17, 2021

closes opensrp/fhircore#40

To build:

  1. Build hapi-fhir-opensrp-security-config module using command mvn clean -X install -Dmaven.buildNumber.skip=true -Dmaven.test.skip=true

@rehammuzzamil rehammuzzamil requested a review from maimoonak May 17, 2021 15:25
@dubdabasoduba dubdabasoduba requested a review from ekigamba May 25, 2021 08:46
@rehammuzzamil rehammuzzamil changed the title WIP - 40 : Added keycloak configuration 40 : Added keycloak configuration May 25, 2021
@rehammuzzamil rehammuzzamil requested a review from ekigamba May 26, 2021 16:11
Comment on lines +6 to +10
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-deployable-pom</artifactId>
<version>5.4.0-PRE5-SNAPSHOT</version>
<relativePath>../../hapi-deployable-pom/pom.xml</relativePath>
</parent>

Choose a reason for hiding this comment

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

I think that this parent causes the artefact to inherit the groupId ca.uhn.hapi.fhir. I also think that this module will be published to Sonatype. We would need to own this groupId ca.uhn.hapi.fhir to publish to it. Therefore, we need to change this to org.smartregister

Copy link
Member

Choose a reason for hiding this comment

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

@ekigamba I agree on this. Do we maintain the versioning too or should we reset it too?

Comment on lines 27 to 60
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-server</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-jpaserver-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-jaxrsserver-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-client</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-client-okhttp</artifactId>
<version>${project.version}</version>
<optional>true</optional>

Choose a reason for hiding this comment

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

Since our approach to Keycloak configuration is not dependent on Hapi FHIR modules, I think that we don't need these

We can also create a new repository for this module. I believe that this is what we follow currently. We ended up splitting the previous OpenSRP server repository into separate and smaller modules each in it's own repository

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I will be removing these unused dependencies of hapi fhir

Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

Copy link
Member

Choose a reason for hiding this comment

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

@ekigamba does opensrp-fhir-authentication sound right for the new module?

Copy link

@ekigamba ekigamba Jun 15, 2021

Choose a reason for hiding this comment

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

@dubdabasoduba Oh! Yea, I am not sure why the fhir work/modules don't have the client/server for each module. But this could also be opensrp-server-fhir-authentication

Copy link
Member

Choose a reason for hiding this comment

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

@ekigamba thanks. Let me follow up on the repo creation

Comment on lines 103 to 113
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-validation-resources-dstu2</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-validation-resources-dstu3</artifactId>
<version>${project.version}</version>
<scope>test</scope>
</dependency>

Choose a reason for hiding this comment

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

These two are not required

Copy link
Author

Choose a reason for hiding this comment

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

Removed @ekigamba

@rehammuzzamil rehammuzzamil requested a review from ekigamba June 15, 2021 11:38
Comment on lines +6 to +10
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-deployable-pom</artifactId>
<version>5.4.0-PRE5-SNAPSHOT</version>
<relativePath>../../hapi-deployable-pom/pom.xml</relativePath>
</parent>
Copy link
Member

Choose a reason for hiding this comment

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

@ekigamba I agree on this. Do we maintain the versioning too or should we reset it too?

Comment on lines 27 to 60
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-server</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-jpaserver-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-jaxrsserver-base</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-client</artifactId>
<version>${project.version}</version>
<optional>true</optional>
</dependency>
<dependency>
<groupId>ca.uhn.hapi.fhir</groupId>
<artifactId>hapi-fhir-client-okhttp</artifactId>
<version>${project.version}</version>
<optional>true</optional>
Copy link
Member

Choose a reason for hiding this comment

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

Agreed.

ca.uhn.fhir.jaxrs: debug
Copy link
Member

Choose a reason for hiding this comment

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

This might be affected if we rename the group id

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.

FHIR authentication POC
3 participants