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

added default db params to dev file (fix) #467

Merged
merged 7 commits into from
Dec 10, 2024

Conversation

arinkulshi-skylight
Copy link
Collaborator

Description

Docker is not connecting to DB anymore. db params are updated to ensure db connection works.

Checklist

  • The title of this PR is descriptive and concise.
  • My changes follow the style guidelines of this project.
  • I have added or updated test cases to cover my changes.
  • I've let the team know about this PR by linking it in the review channel

@jonchang
Copy link
Collaborator

jonchang commented Dec 6, 2024

While this fixes the issue I don't think this is the desired behavior? I think the idea behind the change to begin with was to ensure that when deployed, it would get these values from the environment. Whereas on a local dev build, it should have default values since the azure environment doesn't exist and won't be supplying those values.

@jonchang
Copy link
Collaborator

jonchang commented Dec 6, 2024

% docker compose -f dev-env.yaml up
invalid interpolation format for services.db.environment.POSTGRES_DB.
You may need to escape any $ with another $.
${postgres_db_name:reportvision}

This seems to fix it. I believe it uses bash parameter expansion? https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

diff --git a/dev-env.yaml b/dev-env.yaml
index 14b607e..c384393 100644
--- a/dev-env.yaml
+++ b/dev-env.yaml
@@ -31,12 +31,12 @@ services:
     ports:
       - "5432:5432"
     environment:
-      POSTGRES_DB: ${postgres_db_name:reportvision}
-      POSTGRES_HOST: ${postgres_fqdn:localhost}
-      POSTGRES_USER: ${postgres_user:postgres}
-      POSTGRES_PASSWORD: ${postgres_password:super_secret_password}
-      DB_PORT: ${postgres_port:5432}
-      SSL_MODE: ${postgres_sslmode:disable}
+      POSTGRES_DB: ${postgres_db_name:-reportvision}
+      POSTGRES_HOST: ${postgres_fqdn:-localhost}
+      POSTGRES_USER: ${postgres_user:-postgres}
+      POSTGRES_PASSWORD: ${postgres_password:-super_secret_password}
+      DB_PORT: ${postgres_port:-5432}
+      SSL_MODE: ${postgres_sslmode:-disable}
   api:
     build: 
       context: ./backend

@arinkulshi-skylight
Copy link
Collaborator Author

% docker compose -f dev-env.yaml up
invalid interpolation format for services.db.environment.POSTGRES_DB.
You may need to escape any $ with another $.
${postgres_db_name:reportvision}

This seems to fix it. I believe it uses bash parameter expansion? https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

diff --git a/dev-env.yaml b/dev-env.yaml
index 14b607e..c384393 100644
--- a/dev-env.yaml
+++ b/dev-env.yaml
@@ -31,12 +31,12 @@ services:
     ports:
       - "5432:5432"
     environment:
-      POSTGRES_DB: ${postgres_db_name:reportvision}
-      POSTGRES_HOST: ${postgres_fqdn:localhost}
-      POSTGRES_USER: ${postgres_user:postgres}
-      POSTGRES_PASSWORD: ${postgres_password:super_secret_password}
-      DB_PORT: ${postgres_port:5432}
-      SSL_MODE: ${postgres_sslmode:disable}
+      POSTGRES_DB: ${postgres_db_name:-reportvision}
+      POSTGRES_HOST: ${postgres_fqdn:-localhost}
+      POSTGRES_USER: ${postgres_user:-postgres}
+      POSTGRES_PASSWORD: ${postgres_password:-super_secret_password}
+      DB_PORT: ${postgres_port:-5432}
+      SSL_MODE: ${postgres_sslmode:-disable}
   api:
     build: 
       context: ./backend

You are right looks like I was using the springboot synthax here. I have made those changes.

username: ${DB_USERNAME:postgres_user}
password: ${POSTGRES_USER:postgres_password}
name: ${POSTGRES_DB:postgres_db_name}
url: jdbc:postgresql://${POSTGRES_HOST:localhost}:${DB_PORT:5432}/${POSTGRES_DB:reportvision}?sslmode=${SSL_MODE:disable}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to enable ssl_mode to enforce SSL encryption for this connection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was thinking we can pass SSL_MODE as a argument(enabled,disabled) but kept it disabled by default for connecting to local host. Would this work?

Copy link
Collaborator

@marycrawford marycrawford Dec 10, 2024

Choose a reason for hiding this comment

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

It appears @jonchang and @derekadombek have tested this in the dev-env.yaml and application.yaml as disabled. SSL_MODE enabled adds additional security. However, I will defer to the majority decision of the team on whether it should be enabled at this time or before we hand it off.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok we can set it as enabled by default then.

username: ${DB_USERNAME:postgres_user}
password: ${POSTGRES_USER:postgres_password}
name: ${POSTGRES_DB:postgres_db_name}
url: jdbc:postgresql://${POSTGRES_HOST:localhost}:${DB_PORT:5432}/${POSTGRES_DB:reportvision}?sslmode=${SSL_MODE:disable}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@derekadombek and i tested this in a pairing session today and we found that this was one of the changes needed to get the middleware container image to be able to talk to the database container image when running locally in docker compose. (There may be other changes needed as well, I need to see if I can replicate it locally)

Suggested change
url: jdbc:postgresql://${POSTGRES_HOST:localhost}:${DB_PORT:5432}/${POSTGRES_DB:reportvision}?sslmode=${SSL_MODE:disable}
url: jdbc:postgresql://${POSTGRES_HOST:db}:${DB_PORT:5432}/${POSTGRES_DB:reportvision}?sslmode=${SSL_MODE:disable}

@derekadombek
Copy link
Collaborator

In this PR we might want to add ENTRYPOINT [ "./gradlew", "bootRun", "--continuous", "--args=--server.port=8081" ] to the Dockerfile because right now that command is only being used in docker-compose.

@arinkulshi-skylight
Copy link
Collaborator Author

In this PR we might want to add ENTRYPOINT [ "./gradlew", "bootRun", "--continuous", "--args=--server.port=8081" ] to the Dockerfile because right now that command is only being used in docker-compose.

I added it to the dev-dockerfile as well. Now it is in both places. Let me know if this makes sense or if want me to also remove it from the docker compose file.

@derekadombek
Copy link
Collaborator

In this PR we might want to add ENTRYPOINT [ "./gradlew", "bootRun", "--continuous", "--args=--server.port=8081" ] to the Dockerfile because right now that command is only being used in docker-compose.

I added it to the dev-dockerfile as well. Now it is in both places. Let me know if this makes sense or if want me to also remove it from the docker compose file.

I might have to change it more in the Dockerfile later on once more connections are in place but yeah its better to have the command in the dockerfile itself since not all systems use docker-compose commands.

POSTGRES_USER: ${postgres_user}
POSTGRES_PASSWORD: ${postgres_password}
sslmode: require
POSTGRES_DB: ${postgres_db_name:-reportvision}
Copy link
Collaborator

@marycrawford marycrawford Dec 10, 2024

Choose a reason for hiding this comment

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

Does it matter if the dev-env.yaml is using :-name and the application.yaml file is not? For example, POSTGRES_DB: ${postgres_db_name:-reportvision} in the dev-env.yaml and name: ${POSTGRES_DB:postgres_db_name} in the name: ${POSTGRES_DB:postgres_db_name}.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems one is springboot synthax and the other is bash synthax. app.yaml is being used locally so it should work as is.

@arinkulshi-skylight arinkulshi-skylight added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit bb3edb8 Dec 10, 2024
1 check passed
@arinkulshi-skylight arinkulshi-skylight deleted the add_default_dev_values branch December 10, 2024 23:57
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