-
Notifications
You must be signed in to change notification settings - Fork 723
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
Implement CLI to add members to org configurations #2822
Conversation
27715ca
to
ce7cf68
Compare
/priority important-soon |
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 some suggestions but love it overall! 😍
A couple of more questions about formatting:
- It looks like a diff is generated while yaml (un)marshaling due to sorting keys, whitespace changes. Can you also update the config files with these changes so the next time someone runs this tool the only diff is the username update? Edit: just saw that these are noted in the "caveats" section :)
- Currently, comments are lost when the yaml file is marshalled. Can we use Support (un)marshaling of comments go-yaml/yaml#132 for preserving comments?
cmd/korg/korg.go
Outdated
return fmt.Errorf("reading config: %s", err) | ||
} | ||
|
||
if stringInSlice(config.Members, username) { |
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.
Would need to check if the username exists in config.Admins
as well and display an appropriate error message. For example, korg add nikhita --org kubernetes --confirm
passes now but shouldn't.
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.
Additionally, we'll need to ignore case-sensitivity too. For example, right now Nikhita
and nikhita
are treated as different by stringInSlice
but GitHub treats both as the same username.
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.
Both are taken care of now.
cmd/korg/utils.go
Outdated
} | ||
|
||
func caseAgnosticSort(arr []string) { | ||
sort.Sort(caseAgnostic(arr)) |
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.
Instead of introducing a new type, this can probably be simplified to:
sort.Sort(caseAgnostic(arr)) | |
sort.Slice(arr, func(i, j int) bool { return strings.ToLower(arr[i]) < strings.ToLower(arr[j]) }) |
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.
Done!
cmd/korg/utils.go
Outdated
_, err = r.CommitObject(commit) | ||
if err != nil { | ||
return fmt.Errorf("unable to write commit object to repo: %s", err) |
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.
r.CommitObject
just returns the hash for the commit, right? IIUC it doesn't actually write the commit. The step above (w.Commit
) does 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.
👍🏽
cmd/korg/utils.go
Outdated
return false | ||
} | ||
|
||
func validateOrgs(orgs []string) (valid []string, invalid []string, invalidPresent bool) { |
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 there a reason why we return the valid
orgs? It looks like they aren't being used anywhere. 🤔
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.
Simplified the implementation now. Valid orgs are not returned anymore.
cmd/korg/utils.go
Outdated
sort.Sort(caseAgnostic(arr)) | ||
} | ||
|
||
func parseTeam(team string) (string, string, string, error) { |
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.
Can you add a comment here to document what this function returns?
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.
Removed the team related implementation for now.
cmd/korg/utils.go
Outdated
|
||
org := parts[0] | ||
if !stringInSlice(validOrgs, org) { | ||
return "", "", "", fmt.Errorf("invalid team: %s", team) |
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.
typo...invalid org?
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.
Removed the team related implementation for now.
cmd/korg/utils.go
Outdated
func userInOrg(username string, org string, options Options) bool { | ||
configPath := filepath.Join(options.RepoRoot, fmt.Sprintf(orgConfigPathFormat, org)) | ||
config, err := readConfig(configPath) |
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.
Looks like we need to do the "is user in the org" check for both adding a user to an org and a team. Can we instead read the config once before we call AddMemberToOrgs
and AddMemberToTeams
instead of calling it twice?
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 agree to the suggestion. Reading the config at the start of the command and reusing it makes more sense.
Since the team implementation is deferred to later, we don't need to cache the config in memory for now.
/assign @mrbobbytables @cblecker @nikhita |
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.
@nikhita covered things really well
The big thing I do consider a blocker is the preservation of the comments. Theres a significant amount of metadata there that people use. =/
ce7cf68
to
5a00b84
Compare
I have gone through approaches that we can take to parse YAML comments and write them back. In order to do that we would need to change all structures describing org and team config to store comment data and there is no workaround to that. I did some experimentation here: https://gist.github.com/palnabarun/5e773f8e9e62b467e7cf12affe6591f1. This answer on Stackoverflow beautifully describes the process of loading and dumping YAML files to/from native data structures. The answer also points out that using YAML comments for storing meaningful is not the right approach since YAML is supposed to be a transient data format. Since parsing YAML comments is deemed to be a blocker, I had a look at the number of instances where YAML comments are used to annotate the config. The results are at https://gist.github.com/palnabarun/1fe8b4570607586c391f9190132212be. Because of the above, I feel that spending time on modifying existing data structures to parse YAML comments will provide less value compared to the time invested in the feature. What I propose is:
@nikhita @mrbobbytables WDYT? cc: @MadhavJivrajani |
I will refactor this PR to split out adding members to org and teams. |
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and 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 issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale
…On Wed, 4 May 2022 at 12:37, Kubernetes Triage Robot < ***@***.***> wrote:
The Kubernetes project currently lacks enough contributors to adequately
respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity, lifecycle/stale is applied
- After 30d of inactivity since lifecycle/stale was applied,
lifecycle/rotten is applied
- After 30d of inactivity since lifecycle/rotten was applied, the
issue is closed
You can:
- Mark this issue or PR as fresh with /remove-lifecycle stale
- Mark this issue or PR as rotten with /lifecycle rotten
- Close this issue or PR with /close
- Offer to help out with Issue Triage
<https://www.kubernetes.dev/docs/guide/issue-triage/>
Please send feedback to sig-contributor-experience at kubernetes/community
<https://github.com/kubernetes/community>.
/lifecycle stale
—
Reply to this email directly, view it on GitHub
<#2822 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD24BUG2FCQUIS6DQXV5CJDVIIO2TANCNFSM5AIIXZAA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
5a00b84
to
280d002
Compare
Signed-off-by: Nabarun Pal <pal.nabarun95@gmail.com>
280d002
to
d452c9a
Compare
d452c9a
to
389427a
Compare
Based on our discussions, I have reduced the scope of this PR to include only additions to the org and deferred the addition of users to teams to a future timeline. I have also included the changes to the configuration files in this PR. |
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 tried this command and it's AMAZING!! 😍
Thanks for working on this!
/lgtm
/approve
/hold
I'm good with merging this as-is. Adding a hold in case you want more reviews.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nikhita, palnabarun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
let's merge this? |
/hold cancel |
Fixes: #2812
What is being introduced?
korg
cli to act as a toolkit for operating on organization configurationkorg add
subcommand to add users to orgsSee this note as well: #2822 (comment)
This is a proof of concept implementation with everything put into a single package for simplicity. In future, the functionality can be split into cmd/ and pkg/. Some utilities will need unit tests to ensure behaviour remains consistent with time.
In future, more subcommands can be added. For example, #2575 can be integrated into the korg utility.
Caveats
One-time rejig of the org manifests
Our YAML marshaller/unmarshaller doesn't preserve a custom field order in YAML files that it reads. There is a one-time diff to reformat the config files. Once that is done, the order of fields is consistent across runs. A diff can be found here.
Comments are not preserved
The sigs.k8s.io/yaml library doesn't parse comments. As a result, when these files are modified and save, the comments go away. For example, as seen in this diff. There are around N instances of such comments.
Incomplete support for nested teams
GitHub supports teams inside teams. Currently, the implementation of
korg add
doesn't support it yet. Once this POC goes through one pass of reviews, I will add support for that as well.