-
Notifications
You must be signed in to change notification settings - Fork 69
fix: Properly distribute analytics postgres user #1023
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
Conversation
Fix tests for ocrvs-10263
chore: add markus to e2e users
feat: Trigger notification for events
e2e testing: Notification API requires 'createdAtLocation'
…yconfig into sync-fork-18-09
Sync fork 18 09
…ials' into use-postgres-superuser-credentials
…ials Use postgres superuser credentials
…ials fix shell script env check
…into merge-cc-dev-to-far
Merge cc dev to far
This comment has been minimized.
This comment has been minimized.
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 think we are still using ANALYTICS_POSTGRES_USER in some of the compose files, which should be removed if we are gonna generate the username. As during the deploy, it errors if the ANALYTICS_POSTGRES_USER is not present in the github environment currently
| export EVENTS_APP_POSTGRES_PASSWORD=`generate_password` | ||
| export EVENTS_MIGRATOR_POSTGRES_PASSWORD=`generate_password` | ||
| export ANALYTICS_POSTGRES_PASSWORD=`generate_password` | ||
| export ANALYTICS_POSTGRES_USER=`generate_password` |
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.
So user name is also rotated?
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.
To be honest, I don't know what is generally the convention. In our codebase we sometimes rotate usernames but for Postgres we haven't yet? Feel free to do like you wish.
May help with onboarding if they don't need to decide username for this?
❗ I don't think setup-analytics.sh supports altering usernames
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.
You are right and static user name will work better,
I did tested data seed, deploy, but just imagine situation when you do rotation every time on real environment?
What will happen after 10 sequential deployments?
10 different users with same privileges
I will drop this line, but where should we store analytics user name?
We are using hardcoded user name for events app and migrator
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.
The roles are altered on setup-analytics.sh, no new ones should be created. So that's fine?
Initially I did write the setup-analytics.sh with events_analytics username to keep up with the convention. I'm not sure why Riku changed it to a variable, maybe to help with the same issue why we now replace the variables in core .sql files?
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.
Kept events_analytics user name for consistency with events_app and events_migratior
db7896f to
62d200b
Compare
62d200b to
5fbab1d
Compare
Description
Postgres admin permissions are mixed with analytics user. Goal of this PR is to separate permissions:
Test results
Environment is available at: https://fix-analytics.opencrvs.dev/
Checklist