-
Notifications
You must be signed in to change notification settings - Fork 23
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
Cleanup of the rules depending on users #585
Conversation
mbacovsky
commented
Jan 26, 2024
- missing groups were created in Rover
- users were added to the respective groups
- accesses for some legacy accounts were removed
- missing groups were created in Rover - users were added to the respective groups - accesses for some lagacy accounts were removed
For some changes I'll ask @falox for double check. |
"schema": "(ccx|ccx_sensitive|ccx_srep|ccx_internal|ccx_workloads)", | ||
"owner": true | ||
}, | ||
{ | ||
"group": "ccx-srep-data-access", |
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'm not sure why we gave ccx-srep-data-access and ccx-research-pipeline-trino ownership of the schemas. Is it needed to sync the partitions?
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'm not sure either. It seems safe to remove this to me.
"privileges": ["SELECT"] | ||
}, | ||
{ | ||
"group": "ccx-dev|assisted-lakers|ccx-datalake-access|ceeandpe|na-cs-tam-auto|apac-cs-tam-auto|latam-cs-tam-auto|emea-cs-tam-auto|na-ps-cs-tam-auto|emea-cs-csm-auto|emea-cs-cse-auto|emea-cs-managers|apac-cs-csm-auto|apac-cs-cse-auto|na-cs-csm-auto|na-cs-cse-auto|na-ps-cs-cse-auto|latam-cs-csm-auto|cs-csa-auto-ccx|ccx-pm|telemeter-auth|telemeter-auto-approval|telemeter-manual-approval|cee-sbr-shift|gcs-csm|asr-insights-dashboards", | ||
"schema": "ccx", | ||
"privileges": ["SELECT"] | ||
}, | ||
{ | ||
"user": "superset-ccx", |
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.
@accorvin are these superset accounts still used? I didn't find them in Rover
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.
@mbacovsky no, superset-ccx
is no longer used so this is safe to remove.
"privileges": ["SELECT"] | ||
}, | ||
{ | ||
"user": "(telemetry-automated|telemetry-edmund-abbot|telemetry-analytics)", |
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.
telemetry-automated
is not in Rover neither Edmund Abbot so I skipped those. telemetry-analytics
was added.
LGTM. I would merge next week (Mon 5th), unless @accorvin needs this week |
@accorvin what is the plan with this? Do you prefer to merge and test in Trino first or apply the changes to Starburst only? Is there anything I could do to help with either option? |
@mbacovsky hey, sorry, I forgot abut this one. Everything looks good to me here. I'm inclined to merge this now and let it apply to trino, and I've already given the Starburst team the heads up to pull in these changes too. Give me a final +1 and I'll merge this. |
Ok, lets merge. If we have Friday to monitor and fix eventual issues, +1 from me. |