Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 remove-members helper tool implemented in go #2575
Add remove-members helper tool implemented in go #2575
Changes from 4 commits
c76b933
323261c
1dab05f
9a1daf2
c9a6810
73f657a
6668a0f
9c68029
b38877a
3fe1a19
4e46ddb
7c23c34
8d352b6
2f342c5
686abd3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
@mrbobbytables is the inactive member list a yaml file? how is that generated?
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.
It's been a text file with 1 member per line. The list is generated from this sheet after some manual vetting: https://docs.google.com/spreadsheets/d/1jqxMOo9f1EG72i4sGE7g6RlNPvQ4_qfOx9K-UM4YsLw/edit
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.
Thanks! One more follow up question - how does one generate this sheet?
Want to remove you as a single point of failure :)
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 big stuff to regen it are in the notes tab in the sheet 👍 I thought it was owned/under the sig contribex shared dir, but it looks like I'm the only owner atm. I'll move it over in a bit.
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.
How about simplifying this using
ioutil.ReadFile
as used inremoveMemberFromFile
?Note:
ioutil.ReadFile
has been deprecated in Go 1.16 although it shouldn't be a blocker since this repository still uses Go 1.13.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 be better to
log
here and a more descriptive log messageThere 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.
Should I change all instances of
fmt
tolog
?Currently the output looks like
P.s. I don't really want to remove you 😝 just for showing the output 😹
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 fmt is also ok, but would be good to use a more descriptive message :)
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.
we can probably count how many times
member
occurs in thepath
and return the count. This will ensure thatcount
is accurate.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.
Do you mean to count the number of matches in the said file?
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.
Yeah :)
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.
Not a blocker for this, but potentially something in the future. We can marshal/unmarshal yaml while leaving the comments (go-yaml/yaml#132). This would also let us get team names directly.
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.
@mrbobbytables -- I had a dig at this earlier by parsing the org files using k8s.io/test-infra/prow/config/org.Config, but there seemed to be something off there.
The processed org config when written back to disk had a lot of diff compared to the original file.
We can possibly dig deeper into it later.
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.
It's been a bit since I looked at it , but potentially whitespace =/ since yaml supports some fuzzy inclusion:
is the same as:
Theres what people have written (both formats) and what go will want to format it as.
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.
That as well. we may want to bite the bullet with a big initial diff as the subsequent updates should be deterministic .
One other thing was, in order to process comments, the structs in
k8s.io/test-infra/prow/config/org.Config
should have fields withyaml.Node
as field type and then have post-processing helper method. I had taken a step back because of that complexity.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.
Yeah, what I'd probably do at that point is get away from editing by hand, and then have an add-member function or have a make command that ensures proper formatting before commit. Out of scope for this one though^^;;;
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.
@mrbobbytables -- that's (adding members) what I was trying to automate. 😉
But, yes. Let's talk about this in a separate thread.
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.
Adding to whitespaces and comments related changes, The substantial amount of diff is also created due to the difference in order of the yaml properties.
I was trying to parse the yaml files and removing members in a more structured way, I also faced this.
It seems to be sorting the fields.
I currently implemented this using
kubernetes-sigs/yaml
.