-
Notifications
You must be signed in to change notification settings - Fork 807
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 mailing list members migration functionality #5882
add mailing list members migration functionality #5882
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Priyankasaggu11929 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cc: @cblecker for review. Thanks! 🙂 |
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.
LGTM on a high level for the logic, just one question. Thanks @Priyankasaggu11929!
Edit: We should also add tests for this (as per your TODO): #5882 (comment)
for _, sourceMember := range sourceGroupMembers { | ||
err := as.AddOrUpdateGroupMembers(destinationGroup, sourceMember.Role, []string{sourceMember.Email}) | ||
if err != nil { | ||
logErr := fmt.Errorf("unable to add/update %s to %q as %s: %w", sourceMember.Email, destinationGroup.EmailId, sourceMember.Role, err) | ||
log.Printf("%s\n", logErr) | ||
errs = append(errs, logErr) | ||
continue | ||
} | ||
} |
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.
Is it possible to replace this loop with a single call like such:
sourceMemberEmailIDs := getEmailIDs(sourceGroupMembers)
err := as.AddOrUpdateGroupMembers(destinationGroup, sourceMember, sourceMemberEmailIDs)
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.
Two things here:
-
In the
AddOrUpdateGroupMembers
function, the second argument is taking the member'sRole
.func (as *adminService) AddOrUpdateGroupMembers(group GoogleGroup, role string, members []string) error {...}
-
So, we're using a loop to process one Mailing List member at a time from
sourceGroupMembers
list.
We pass theirRole
andEmail
toAddOrUpdateGroupMembers()
to update the destination group without affecting their existing role.err := as.AddOrUpdateGroupMembers(destinationGroup, sourceMember.Role, []string{sourceMember.Email})
Please let me know if I misunderstood your suggestion.
@Priyankasaggu11929 for testing - we should probably update the groups fake client: https://github.com/kubernetes/k8s.io/blob/main/groups/fake/fake_client.go and then test the high level behaviour. Agreed that behaviour with actual API calls can't really be tested, but the fake client comes close. |
27edbee
to
17c3014
Compare
added tests in the latest commit refresh. @MadhavJivrajani, PTAL again. Thanks for the suggestions! 🙂 |
@Priyankasaggu11929 How do you expect this to be run? Is it something a user (who presumably has credentials with sufficient permissions) runs on their local system? The thought process I have looking at this:
|
Hello @cblecker, thanks for the review!
To run this, the idea is to use the new Since the usecase is to migrate a finite number of ML (~34), I'm thinking of just hard-coding source/destination ML names in the prow job yaml itself and updating them as needed.
Owners/managers will be declared as yaml, as in the existing OWNERS file.
IMU, The new This is my understanding atm w/o actual API call testing, but lmk if it works differently.
But the rate limiting part, I'll only be able to test once we add the prow job and have a few runs of it. Will improve accordingly from the test runs. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale Ping @cblecker @MadhavJivrajani for another round of review. 🙂 |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
@palnabarun, @mrbobbytables – could you help (with steering elevated permissions) to perform the mailing list migrations? I'll then close this PR in favor of that. Thanks! |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Description
This PR attempts to add functionality for migrating mailing list members across two google groups.
add
MigrateMailingListMembers
function ingroups/service.go
to implement the migration logicadd
PerformMailingListMigration function
function and new flags (--migrate
,--destinationGroup
,--sourceGroup
) ingroups/service.go
to trigger mailing list members migrationadd a new Makefile target –
migrate-members
to execute the migration with specified source and destination groups as:add test
TestMigrateMailingListMembers
ingroups/service_test.go
Note:
I haven't been able to test it yet due to having no credentials for API calls.
Once it's reviewed and merged, I'll try it in a prow job similar to existing ones.
/assign @mrbobbytables @MadhavJivrajani (for early feedback)
/sig contributor-experience