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

WIP: add guiding policy for contribex review on test-infra repo #7931

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Priyankasaggu11929
Copy link
Member

@Priyankasaggu11929 Priyankasaggu11929 commented Jun 7, 2024

Follow up item from kubernetes/test-infra#32681

As a follow up, they (ContribEx TLs) are working on a more descriptive policy for changes to prow contribution workflows so that there is more clarity in this space.

/assign @cblecker @MadhavJivrajani @BenTheElder


Reviewers note

This policy document is pretty much an early draft, and possibly listing more areas than what actually falls under ContribEx.

(Please highlight areas that needs to be removed from the list along with all other review comments. Thank you.)

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 7, 2024
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 7, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Priyankasaggu11929

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2024
@Priyankasaggu11929
Copy link
Member Author

/hold for reviews

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2024
@Priyankasaggu11929 Priyankasaggu11929 force-pushed the contribex-owners-policy branch 2 times, most recently from cd93bb5 to 4b8e757 Compare June 7, 2024 17:06
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani left a comment

Choose a reason for hiding this comment

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

Thanks for starting this @Priyankasaggu11929 ❤️
Leaving a few initial comments!

A review from SIG Contributor Experience is necessary if any changes introduced by a pull request to the test-infra repo:

- Introduce changes to how humans interact with Prow and its components (changes that alter contributor user experience)
- Introduce changes to how bot interact with Prow? (TODO: verify!)
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm not sure if this falls under the purview of ContribEx. It would help to narrow the scope down to aspects that end up being surfaced to contributors themselves. For example:

  • If there is a change in how the bot handles webhooks, that's probably better handled by SIG Testing considering they own that code and have expertise in it.
  • If there is a change that introduces retries, but as a side result of that, the bot ends up commenting more or less than it did before, then that is something that we should be chiming in on.

Copy link
Member

Choose a reason for hiding this comment

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

If there is a change that introduces retries, but as a side result of that, the bot ends up commenting more or less than it did before, then that is something that we should be chiming in on.

Presumably a change like this would only introduce more bot comments if there is a bug, so this change wouldn't need review either.

I would expect under the current unwritten policy that a change like "prow now automatically closes PRs without CLA after 5 days" to require contribex review of the proposed workflow and any technical details to remain under whatever project implements it (so probably prow/plugins technical owners), while "prow now handles transient github failures better" to remain SIG Testing / Prow owners.


#### Plugins under ContribEx Review

Following plugins fall under ContribEx review:
Copy link
Contributor

@MadhavJivrajani MadhavJivrajani Jun 14, 2024

Choose a reason for hiding this comment

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

If we are scoping plugins under ContribEx's purview, we should probably also include the easter-eggy plugins here as well like:

  • shrug
  • joke
  • cat
  • dog
  • pony
  • shrug
  • heart

Mainly because some of these pick images/content randomly from a pre-existing set and we want to make sure that set deosn't change, and if it does, it is an acceptable change.

There's also plugins like sigmention and WIP that are contributor facing.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think trying to enumerate the plugins makes sense.

Any plugin can introduce contributor facing behavior changes, and making changes to any of these plugins can not involve a contributor workflow change (e.g. a simple bug fix).

I would again argue that all of these plugin implementations are owned by SIG testing while contributor experience has previously asserted ownership over the contributor workflow (and similarly googlegroups automation is owned by SIG K8s Infra while contribex has asserted some ownership over some of the groups).

What we lack currently is anything written that actually says "contributor experience owns contributor workflows and you need to get approval for workflow changes" with some examples.

If you're going to unilaterally own implementations, OWNERS files and subprojects are the correct way to declare this, not another doc in k/community.


---

### Changes to `Label_sync` Path
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
### Changes to `Label_sync` Path
### Changes to `label_sync` Path


---

## Out of Scope
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

... I'd sort of expect documents about test-infra specific changes to be in test-infra?

Sections like the list of files seem very likely to get out of date across repos

- All files under [kubernetes/org/](https://github.com/kubernetes/test-infra/tree/master/config/jobs/kubernetes/org) path
- [kubernetes/sig-k8s-infra/trusted/sig-contribex-k8s-triage-robot.yaml](https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-k8s-infra/trusted/sig-contribex-k8s-triage-robot.yaml)
- [kubernetes/sig-k8s-infra/trusted/sig-contribex-peribolos.yaml](https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-k8s-infra/trusted/sig-contribex-peribolos.yaml)
- [kubernetes/sig-k8s-infra/trusted/sig-k8s-infra-groups.yaml](https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-k8s-infra/trusted/sig-k8s-infra-groups.yaml)
Copy link
Member

Choose a reason for hiding this comment

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

Erm, this is SIG K8s Infra? I wouldn't expect changes to keeping this automation running to require input from other sigs, this is a technical implementation.


### Changes to Prow CI Jobs and Their OWNERS Files

Changes to the definition of the following Prow CI jobs files as well as their respective OWNERS files, owned directly or indirectly by SIG ContribEx:
Copy link
Member

Choose a reason for hiding this comment

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

Many of these changes are going to be technical in nature.

Instead, what I'm looking for is a document outlining what it is that makes a change something SIG Contribex should review, I.E. "changes that impact contributor workflows / contributor facing behaviors need contribex tech lead review" or something like that.

It is not realistic to unilaterally review all changes to all of these files, plenty of changes will just be e.g. updating to the latest builds and should not require bottlenecking on contribex TLs.

But right now if I tell people "you need contribex review for this contributor facing workflow change" there's no actual written reference for this, and I'm not sure how many people in the project are aware of any such policy.

@BenTheElder
Copy link
Member

Thanks for the draft! I don't think this is quite what I was expecting 😅 , commented more above

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages 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 PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants