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.

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

Use excludedRepos for kubernetes org tide query #10846

Closed
spiffxp opened this issue Jan 19, 2019 · 11 comments · Fixed by #10919
Closed

Use excludedRepos for kubernetes org tide query #10846

spiffxp opened this issue Jan 19, 2019 · 11 comments · Fixed by #10919
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience.
Milestone

Comments

@spiffxp
Copy link
Member

spiffxp commented Jan 19, 2019

AKA switch to using org: kubernetes -repo:kubernetes/kubernetes with the excludedRepos field
per @cjwagner's suggestion in #10569 (review)

/assign @cjwagner
because you suggested it... or maybe this is for whomever is test-infra on call?

/assign @amwat
you'll want to double check whether this means any updates to the test-infra playbook for release-team, I don't think so? but I'm being too lazy to check while writing this

@spiffxp spiffxp added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 19, 2019
@spiffxp
Copy link
Member Author

spiffxp commented Jan 19, 2019

/milestone v1.14

@cjwagner
Copy link
Member

cjwagner commented Jan 23, 2019

22 of the 67 Kubernetes repos are not using Tide today:

kubernetes/api
kubernetes/apiextensions-apiserver
kubernetes/apimachinery
kubernetes/apiserver
kubernetes/cli-runtime
kubernetes/cloud-provider
kubernetes/cluster-bootstrap
kubernetes/code-generator
kubernetes/component-base
kubernetes/csi-api
kubernetes/csi-translation-lib
kubernetes/kompose
kubernetes/kube-aggregator
kubernetes/kube-controller-manager
kubernetes/kube-proxy
kubernetes/kube-scheduler
kubernetes/kubelet
kubernetes/metrics
kubernetes/node-api
kubernetes/sample-apiserver
kubernetes/sample-cli-plugin
kubernetes/sample-controller

Special note: We already have Tide configured for kubernetes/komposer, but that appears to be a typo and should be kubernetes/kompose.

We have a couple ways to move forward with this.

  1. We can switch the Tide config to specify org: kubernetes and explicitly exclude the above repos from the query using excludedRepos. This would be a no-op change for all existing repos and would mean that any newly created repo will have Tide enabled by default. We can then incrementally opt-in the excluded repos until all repos use Tide.
  2. We can keep the Tide config as is and work on on-boarding the remaining repos to Tide. Newly created repos will not have Tide enabled. Once all repos use Tide we can switch to using the org: kubernetes query at which point newly created repos will default to having Tide enabled.
  3. We can switch the Tide config to org:kubernetes and onboard all the remaining repos to Tide all at once. Lets avoid this option if possible.

I think the first option is the best?

edit: accidentally included "kubernetes/kubernetes" for which Tide is certainly already enabled.

@cjwagner
Copy link
Member

/sig contributor-experience

@k8s-ci-robot k8s-ci-robot added the sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. label Jan 23, 2019
@spiffxp
Copy link
Member Author

spiffxp commented Jan 23, 2019

With the exception of kubernetes/kompose, all of the repos listed are staging repos (https://github.com/kubernetes/kubernetes/tree/master/staging)

They don't want any PR's to land there, instead redirecting to encourage PR's to k/k/staging

@spiffxp
Copy link
Member Author

spiffxp commented Jan 23, 2019

/assign @nikhita @sttts
There is concern that enabling tide on these staging repos could one day result in a PR being merged to those repos if someone ignored the PR template and opened a PR anyway, and someone else approved it. So, we could keep maintaining a list of staging repos and exclude them from tide's query.

But, I feel like we have sufficient safeguards in place today:

I sorta want to understand if we can instead whittle the list down to nothing and reduce ongoing maintenance burden.

@spiffxp
Copy link
Member Author

spiffxp commented Jan 23, 2019

/assign @caesarxuchao
same question

@cblecker
Copy link
Member

Hmm.. would a blockade configuration for those repos be a good idea? Just blockade on any PR?

@spiffxp
Copy link
Member Author

spiffxp commented Jan 23, 2019

I guess I was hoping to avoid ongoing maintenance but fair, blockade would be my recommendation if we need another safeguard.

@caesarxuchao
Copy link
Member

It would be ideal if we can instruct Tide to block PRs by setting up some special files in the staging repos. Does Tide support that?

@nikhita
Copy link
Member

nikhita commented Jan 23, 2019

There is concern that enabling tide on these staging repos could one day result in a PR being merged to those repos if someone ignored the PR template and opened a PR anyway, and someone else approved it.

I think if someone is listed as an approver for a staging repo, they know not to approve PRs directly to the published repo....but +1 about having safeguards in place.

If we don't want to use excludedRepos, then blockade seems good to me.

@cjwagner
Copy link
Member

If we don't want Tide to merge PRs to a repo we should just use excludedRepos to make Tide ignore the repos entirely. I don't see any downside to using excludedRepos in this case. The motivation for enabling Tide across the org is twofold:

  1. Provide a consistent PR experience across the org.
  2. Automatically enable Tide on newly created repos in the org.

I think we only really care about minimizing the number of excludedRepos for the sake of providing a consistent PR experience. Staging repos shouldn't receive normal (or any) PRs so providing a consistent workflow is irrelevant and it makes sense to list these as excludedRepos.

Blockade or any other mechanisms that block PRs from merging still require Tide to search for PRs in those repos, process them, and post statuses to them. The GH API is already struggling with the size of our requests so we should try to avoid increasing load in cases where we have a more efficient alternative that is sane.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience.
Projects
None yet
8 participants