Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Extend subproject and repo generation for generator app #2731

Closed
wants to merge 1 commit into from
Closed

Extend subproject and repo generation for generator app #2731

wants to merge 1 commit into from

Conversation

justaugustus
Copy link
Member

@justaugustus justaugustus commented Sep 30, 2018 •
edited
Loading

Adds a Repo struct and additional fields to the Subproject and Group structs of the generator app.

This adds some granularity to allow you to define:

  • Repos / subdirectories not owned by a SIG Subproject
  • Subproject ChairsContacts
  • Repo names / descriptions / URLs / OWNERS

...or decide to continue using the OWNERS files for subprojects. :)

I've created a gist that you can pull to test or view an example of the structure:

It also cleans up some syntax nits.

Fixes: #2619
Related: #1913

/kind feature
/sig contributor-experience

Signed-off-by: Stephen Augustus foo@agst.us

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/aws area/provider/azure Issues or PRs related to azure provider ug/big-data Categorizes an issue or PR as relevant to ug-big-data. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Sep 30, 2018
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Sep 30, 2018
@k8s-ci-robot k8s-ci-robot added sig/gcp area/provider/ibmcloud Issues or PRs related to ibmcloud provider sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/multicluster Categorizes an issue or PR as relevant to SIG Multicluster. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. area/provider/openstack Issues or PRs related to openstack provider sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Sep 30, 2018
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2018
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2018
@justaugustus
Copy link
Member Author

@timothysc -- Fixed up.
Setting a lazy consensus timebox for Friday, November 9, 5pm PT.

@cblecker
Copy link
Member

cblecker commented Nov 6, 2018

I'd like an explicit thumbs up directionally from @spiffxp (as he placed the hold) before we move forward with any sort of lazy consensus.

@justaugustus
Copy link
Member Author

That's fine, @cblecker. I'll ping @spiffxp later today.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2018
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 9, 2018
@timothysc timothysc requested review from timothysc and removed request for timothysc November 13, 2018 18:59
spiffxp
spiffxp reviewed Nov 30, 2018
View reviewed changes
Copy link
Member

@spiffxp spiffxp left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the whole I still feel like this doesn't provide much benefit for the additional complexity it introduces.

Also the Subproject.Chairs change didn't show up in my files view during review, but I'm pretty opposed to that. My suggested Owners compromise is what this should be instead.

generator/app.go
@@ -109,6 +120,7 @@ type Group struct {
Leadership LeadershipGroup `yaml:"leadership"`
Meetings []Meeting
Contact Contact
Repos []Repo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opposed to this:

  • working groups don't own code
  • sigs don't own code directly, they own subprojects that own code

generator/app.go
// Subproject represents a specific subproject owned by the group
type Subproject struct {
Name string
Description string
Label string
Chairs []Person
Repos []Repo
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm opposed to this. Take the existing owners list and convert it into repos or subdirs via a helper function for display if you want, but don't add the complexity here.

generator/app.go
Owners []string
Meetings []Meeting
Contact *Contact
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with this.

generator/app.go
// Subproject represents a specific subproject owned by the group
type Subproject struct {
Name string
Description string
Label string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this should be omitempty

generator/app.go
// Subproject represents a specific subproject owned by the group
type Subproject struct {
Name string
Description string
Label string
Repos []Repo
Owners []string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here's an example of how you could add the human info, while preventing it from falling out of data with the OWNERS files:

  • change this to OwnersFiles []string
  • add a field Owners []Person
  • update app/generator to verify that the people listed in Owners are actually present in the OwnersFiles as approvers

You have to do that third step though. If you don't, then you're left with two sources of truth as to who has the final say, and that's more confusing than the situation today.

@nikhita nikhita mentioned this pull request Dec 7, 2018
Closed
@cblecker cblecker mentioned this pull request Jan 7, 2019
Merged
@justaugustus
Extend subproject and repo generation for generator app
7826d87 
Signed-off-by: Stephen Augustus <foo@agst.us>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: justaugustus
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: cblecker

If they are not already assigned, you can assign the PR to them by writing /assign @cblecker in a comment when ready.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
@nikhita
Copy link
Member

nikhita commented Mar 12, 2019

For the record: this is currently blocked on Steering, since we need docs for what a subproject "lead" means and the processes around it before we can add fields for it in sigs.yaml.

Ref: kubernetes/steering#36 (comment)

@nikhita nikhita mentioned this pull request Mar 17, 2019
Closed
@spiffxp
Copy link
Member

spiffxp commented May 7, 2019

/close
This is in dire need of rebasing. I've been leaving this open as a reminder that it's out here, but I don't see us getting to this anytime soon. Please re-open if you disagree

We will come back to this via https://github.com/kubernetes/community/issues/1673#issuecomment-484746226

@k8s-ci-robot
Copy link
Contributor

@spiffxp: Closed this PR.

In response to this:

/close
This is in dire need of rebasing. I've been leaving this open as a reminder that it's out here, but I don't see us getting to this anytime soon. Please re-open if you disagree

We will come back to this via https://github.com/kubernetes/community/issues/1673#issuecomment-484746226

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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timothysc timothysc timothysc left review comments

@spiffxp spiffxp spiffxp left review comments

Assignees

@spiffxp spiffxp

@cblecker cblecker

Labels
area/provider/azure Issues or PRs related to azure provider area/provider/ibmcloud Issues or PRs related to ibmcloud provider area/provider/openstack Issues or PRs related to openstack provider area/provider/vmware Issues or PRs related to vmware provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. committee/steering Denotes an issue or PR intended to be handled by the steering committee. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/feature Categorizes issue or PR as related to a new feature. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/multicluster Categorizes an issue or PR as relevant to SIG Multicluster. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/scalability Categorizes an issue or PR as relevant to SIG Scalability. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/service-catalog Categorizes an issue or PR as relevant to SIG Service Catalog. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. sig/ui Categorizes an issue or PR as relevant to SIG UI. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. ug/big-data Categorizes an issue or PR as relevant to ug-big-data.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subproject details in sigs.yaml
7 participants
@justaugustus @cblecker @spiffxp @andrewsykim @timothysc @k8s-ci-robot @nikhita