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

Fix: local deployment issues #72

Merged
merged 15 commits into from
May 16, 2024
Merged

Fix: local deployment issues #72

merged 15 commits into from
May 16, 2024

Conversation

nitin-vavdiya
Copy link
Contributor

@nitin-vavdiya nitin-vavdiya commented May 13, 2024

Description

Changes:

  1. Spring docker-compose support added
  2. Spring openAPI support added for swagger
  3. Spring boot version updated
  4. prettier plugin and prettier java version updated
  5. fix: application.yaml was not loaded when we started the application using IDE
  6. fix: flyway migration was not running when we started the application using IDE
  7. Local installation guide update for docker-compose support
  8. Duplicate dependency for JPA is removed

fix: #71

Pre-review checks

Please ensure to do as many of the following checks as possible, before asking for a committer review:

@carslen
Copy link
Contributor

carslen commented May 13, 2024

@nitin-vavdiya Currently the Eclipse Contributor Agreement (ECA) check fails because @ravi-ghadiya has not signed the ECA.

Latest dependency changes added to DEPENDENCY_BACKEND
@nitin-vavdiya nitin-vavdiya requested a review from carslen May 13, 2024 08:12
Copy link
Contributor

@carslen carslen left a comment

Choose a reason for hiding this comment

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

As this PR has > 1000 lines of changed code, Eclipse Foundation IP-Check ticket opened: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/14720

@ClosedSourcerer please review code and approve PR if changes are valid.

@kishoreinvits
Copy link

kishoreinvits commented May 15, 2024

@nitin-vavdiya, Great job. Thank you for doing this.

It will be super helpful to update document Local Development Install so that any future developers can get started using your work.

With more variables added, helm chart also needs an update. I think it exceeds scope of this PR. So added new issue #73 for that.

set -e

psql -v ON_ERROR_STOP=1 --username "$POSTGRES_USER" --dbname "$POSTGRES_DB" <<-EOSQL
CREATE DATABASE auth;

Choose a reason for hiding this comment

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

Consider creating if not exists, so that its idempotent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b3a7dbc

@@ -36,17 +47,28 @@ spring:

flyway:
enabled: true
url: jdbc:postgresql://${DCM_DATASOURCE_HOST:localhost:5432}/${DCM_DATASOURCE_NAME:dcm}
url: jdbc:postgresql://${DCM_DATASOURCE_HOST:localhost:15432}/${DCM_DATASOURCE_NAME:dcm}
Copy link

@kishoreinvits kishoreinvits May 15, 2024

Choose a reason for hiding this comment

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

Why did you have to change port. Even docker-compose you added is not using these ports. Local development install is also recommending these standard ports. Recommend switching back to standard ports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b3a7dbc

@@ -54,7 +54,7 @@
<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-webflux</artifactId>
<version>3.1.5</version>
<version>3.2.5</version>
</dependency>

<dependency>

Choose a reason for hiding this comment

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

As you are touching this file, can you please remove duplicate dependency in line 82 though you did not add it

<dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-data-jpa</artifactId>
        </dependency>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b3a7dbc


@Configuration
@AllArgsConstructor
public class OpenApiConfig {

Choose a reason for hiding this comment

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

When I tried to run app, project fails to initialize this class. Please check if you also see this or update recommended local set up.

I am using openJDK 17

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No we tested with JDK 17 only

Choose a reason for hiding this comment

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

Can you briefly update document Local Development Install. I will try again.

Copy link
Contributor Author

@nitin-vavdiya nitin-vavdiya May 16, 2024

Choose a reason for hiding this comment

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

There was one annotation missing on the spring boot main class

README is also updated

Fixed with b3a7dbc

@ClosedSourcerer
Copy link
Contributor

As this PR has > 1000 lines of changed code, Eclipse Foundation IP-Check ticket opened: https://gitlab.eclipse.org/eclipsefdn/emo-team/iplab/-/issues/14720

@ClosedSourcerer please review code and approve PR if changes are valid.

From my POV the changes seem sensible and improve DCMFOSS.
I asked @kishoreinvits to have a look at it as well.
He is one of the technical coaches for the DCM FOSS stream at the Tractus-X Community Days.

As soon as his concerns are taken care off this would be good to merge, in my humble opinion.

![Postgres connection](images/dev/5.png "Docker postgres connection")
On startup, the application also creates a Keycloak container based on the configurations in the Compose file. Initial configurations, including the creation of realms, clients, and users, are performed using the `dcm_realm.json` file.

![Docker desktop running containers](images/dev/11.png)

Choose a reason for hiding this comment

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

Prefer not to add docker desktop reference. See #68 . But in this case, it is only representation.

Copy link

@kishoreinvits kishoreinvits left a comment

Choose a reason for hiding this comment

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

@ClosedSourcerer - Looks good to me. Especially adding missing annotation.

@carslen
Copy link
Contributor

carslen commented May 16, 2024

Waiting to finish EF IP-Check with due date 22.05.2024. After IP-Check succeded I'll approve and merge PR.

@stephanbcbauer stephanbcbauer merged commit 7019114 into eclipse-tractusx:main May 16, 2024
3 checks passed
@carslen
Copy link
Contributor

carslen commented May 16, 2024

@stephanbcbauer why has this PR been merged, ignoring the changes requested? In a way this is a no-go and disrespects the work I've spend into this PR.

EF IP-Check for this PR is still open. I've stated clearly that I'll merge this PR as soon as the IP-Check has been successfully completed.

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.

Sad sad Panda (Records on maven-prettier)
6 participants