-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add cronjob to backfill corporate partner user groups' memberships and classifications #57
Conversation
…scripts where used for
…b kubernetes template
…ay.new vs times.map per hound
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.
Couple things, nothing blocking. Also, kubernetes/cron_sync.yml
filename could be more descriptive even though there's only one (so far...)
I assume these scripts were tested by running them locally. Do you want a way to run this manually via github action? An additional workflow with the workflow_dispatch (a la manual_sync.yml over at listmonk-sync) would do that and keep logs.
def memberships_insert_query(memberships_to_create) | ||
# Values is a string that will look like ($1, $2, $3, $4), ($5, $6, $7, $8), ..etc.. | ||
values = Array.new(memberships_to_create.length) do |i| | ||
"($#{(4 * i) + 1}, $#{(4 * i) + 2}, $#{(4 * i) + 3}, $#{(4 * i) + 4})" |
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.
This feels like kind of a kludgy way to build a parameterized query. Since this doesn't take any input, I don't feel like you gotta use conn.exec_params
instead of just building the query string directly. Even though it took me second to parse this, I'm still only like 3/10 on it.
I was going back and forth whether we wanted a manual sync as well. IMO, I can see someone asking backend devs to run this manually on a per user group basis but not for all corp partners. (Then it would probably be easier to do locally for the specific user group). BUT it doesnt hurt to add it anyway and in this way I can test this runs as soon as it gets merged and deployed (as opposed to waiting for cron schedule) |
@zwolf absolutely no rush. I re-requested review since i added manual sync ability as well and wanted a second eye. |
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.
Left suggestions for the most up to date versions of the shared actions used by this workflow.
Co-authored-by: Zach Wolfenbarger <[email protected]>
Co-authored-by: Zach Wolfenbarger <[email protected]>
Co-authored-by: Zach Wolfenbarger <[email protected]>
Co-authored-by: Zach Wolfenbarger <[email protected]>
Co-authored-by: Zach Wolfenbarger <[email protected]>
Add cronjob to backfill corporate partner user groups' memberships and classifications.
I'm not entirely confident on cron_sync.yml, but followed example of https://github.com/zooniverse/listmonk-sync.
Added comments on old backfill scripts that were used when copying over panoptes/talk classifications to ensure there is context on what those scripts were and how they were used.
Unfortunately with panoptes ruby client, we cannot create memberships to user_groups even with admin flag turned on (unless we are associated with the user_group). Since we needed read access anyway, included write access to write memberships. (DM me if we want to change the panoptes access string).