-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1963 - Database Name prefixed #2152
Conversation
@@ -88,7 +88,7 @@ env: | |||
QUEUE_PREFIX: ${{ secrets.QUEUE_PREFIX }} | |||
API_PORT: ${{ secrets.API_PORT }} | |||
WEB_PORT: ${{ secrets.WEB_PORT }} | |||
DATABASE_NAME_KEY: ${{ secrets.DATABASE_NAME_KEY }} | |||
DATABASE_NAME_PREFIX: ${{ secrets.DATABASE_NAME_PREFIX }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need the prefix? DATABASE_NAME could be something which is mapped from github env to deployment directly right?
like this DATABASE_NAME: ${{ vars.DATABASE_NAME }}
Also should database name be in secrets in github? we can use vars
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can have and we already discussed this i guess. The thing is even without the database_name or the database_name_prefix in the github secrets, the database name should be taken as SIMSDB and all the actions should work. This change is for an exception scenario where we have to create a new set of db for new environment. So introduced the prefix, rather going with the database_name and repeating with the same value everywhere.
This inturn does not need the change in the prod environment deployment, if the secret database_name_key is removed from the Prod github secrets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can talk as a team and settle this. Maybe I am missing something but I was under the assumption that the DB name would go under the sims-env-creds maybe, but the current approach would also be acceptable.
@@ -103,6 +103,7 @@ jobs: | |||
echo Git Red: ${{ inputs.gitRef }} | |||
echo BUILD NAMESPACE: $BUILD_NAMESPACE | |||
echo HOST_PREFIX: $HOST_PREFIX | |||
echo DATABASE_NAME_PREFIX: $DATABASE_NAME_PREFIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an echo but we need the same for deploy-sims-api, workers and queue-consumers right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but i guess, we can remove all the echos in the deploy-sims-api, workers and queue-consumers too, as they depends on run-db-migrations. its okay to have at one place :). I will add it in other places too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay then we can remove them in all the entires.
Great job on having the staging environment ready. Added 2 comments out of which only first one is something I am confused about. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, let's try to settle the comments quickly 😉
I do not see a major concern between the options.
@@ -103,6 +103,7 @@ jobs: | |||
echo Git Red: ${{ inputs.gitRef }} | |||
echo BUILD NAMESPACE: $BUILD_NAMESPACE | |||
echo HOST_PREFIX: $HOST_PREFIX | |||
echo DATABASE_NAME_PREFIX: $DATABASE_NAME_PREFIX |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -88,7 +88,7 @@ env: | |||
QUEUE_PREFIX: ${{ secrets.QUEUE_PREFIX }} | |||
API_PORT: ${{ secrets.API_PORT }} | |||
WEB_PORT: ${{ secrets.WEB_PORT }} | |||
DATABASE_NAME_KEY: ${{ secrets.DATABASE_NAME_KEY }} | |||
DATABASE_NAME_PREFIX: ${{ secrets.DATABASE_NAME_PREFIX }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can talk as a team and settle this. Maybe I am missing something but I was under the assumption that the DB name would go under the sims-env-creds maybe, but the current approach would also be acceptable.
@@ -101,8 +100,6 @@ jobs: | |||
run: | | |||
echo Git Environment: ${{ inputs.environment }} | |||
echo Git Red: ${{ inputs.gitRef }} | |||
echo BUILD NAMESPACE: $BUILD_NAMESPACE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did we remove the echo of the build_namespace ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad missed it in the printing below, reverted the change back :)
@@ -276,8 +276,10 @@ parameters: | |||
value: superuser-username | |||
- name: DB_PASSWORD_KEY | |||
value: superuser-password | |||
- name: DATABASE_NAME_KEY | |||
required: true | |||
- name: API_SECRET_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API_SECRET_NAME is not required to be added here. QUEUE_CONSUMERS_SECRET_NAME
can be used.
key: ${DATABASE_NAME_KEY} | ||
name: ${DB_SECRET_NAME} | ||
key: ${SIMS_DB_NAME} | ||
name: ${API_SECRET_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
QUEUE_CONSUMERS_SECRET_NAME
can be used.
devops/openshift/workers-deploy.yml
Outdated
@@ -124,8 +124,10 @@ parameters: | |||
value: superuser-username | |||
- name: DB_PASSWORD_KEY | |||
value: superuser-password | |||
- name: DATABASE_NAME_KEY | |||
required: true | |||
- name: API_SECRET_NAME |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API_SECRET_NAME is not required to be added here. WORKERS_SECRET_NAME
can be re-used.
devops/openshift/workers-deploy.yml
Outdated
key: ${DATABASE_NAME_KEY} | ||
name: ${DB_SECRET_NAME} | ||
key: ${SIMS_DB_NAME} | ||
name: ${API_SECRET_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WORKERS_SECRET_NAME
can be used.
@@ -88,7 +88,6 @@ env: | |||
QUEUE_PREFIX: ${{ secrets.QUEUE_PREFIX }} | |||
API_PORT: ${{ secrets.API_PORT }} | |||
WEB_PORT: ${{ secrets.WEB_PORT }} | |||
DATABASE_NAME_KEY: ${{ secrets.DATABASE_NAME_KEY }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the BUILD_NAMESPACE
already available and used, for instance, in .github\workflows\env-setup-build-forms-server.yml
We can do the same for the KEYCLOAK_REALM
.
By the way, @dheepak-aot for these ones that were hard-code, we used the vars
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a repository variable and changed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the additional refactors to replace the tools namespace and keycloak realm. 👍
devops/openshift/workers-deploy.yml
Outdated
@@ -137,6 +137,7 @@ parameters: | |||
- name: BUILD_TAG | |||
value: "0" | |||
- name: WORKERS_SECRET_NAME | |||
value: sims-api-creds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not following this default value. Do we need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following the steps follwed in api-deploy.yml, also the name was not self understanding what it is, the value just gives an idea what this secret name is.
@@ -289,6 +289,7 @@ parameters: | |||
- name: BUILD_TAG | |||
value: "0" | |||
- name: QUEUE_CONSUMERS_SECRET_NAME | |||
value: sims-api-creds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following the steps follwed in api-deploy.yml, also the name was not self understanding what it is, the value just gives an idea what this secret name is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, the standard is old and does not represent well the change that we did when we introduced the sims-ENV-creds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the changes, only one last comment to be checked.
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing the changes, looks good 👍
Please ensure that we have the instructions updated in the next release instructions
https://app.zenhub.com/workspaces/student-information-management-system-5fce9df5aa1b45000e937014/reports/release?release=Z2lkOi8vcmFwdG9yL1JlbGVhc2UvOTQxODU
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good work @guru-aot
Note: Connecting the existing patroni locally and creating a database in the name '{sims_db_name}' is required as a prerequisite, so that the github actions can find the new database when run.