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

Update the OWNERS for the kubeadm documentation #8506

Closed
wants to merge 1 commit into from

Conversation

luxas
Copy link
Member

@luxas luxas commented May 11, 2018

The same thing as kubernetes/kubernetes#63551, for this repo
/assign @timothysc @heckj

@heckj can you confirm that owners aliases work in this repo?

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 11, 2018
@k8sio-netlify-preview-bot
Copy link
Collaborator

Deploy preview for kubernetes-io-master-staging ready!

Built with commit 3369c96

https://deploy-preview-8506--kubernetes-io-master-staging.netlify.com

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2018
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@mikedanese
Copy link
Member

/lgtm

@heckj
Copy link
Contributor

heckj commented May 12, 2018

@timothysc @luxas - yep, owners definitely operational within the kubernetes/website repo - separate sets for the blog, different areas, etc. All this seems fine to me, and would like @zacharysarah to give a thumbs up here as well.

/assign @zacharysarah

@luxas
Copy link
Member Author

luxas commented May 18, 2018

ping @heckj @zacharysarah
cc @mistyhacks

@mdlinville
Copy link
Contributor

/assign @chenopis

Zach is on vacation.

Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @chenopis 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

@zacharysarah
Copy link
Contributor

zacharysarah commented May 31, 2018

@luxas 👋 Sorry for the delay, I was out on vacation. A few things:

  1. Thanks for updating the OWNERS_ALIASES entry for sig-cluster-lifecyle! 🙇

  2. I'm /holding this PR to give @kubernetes/sig-docs-maintainers a chance to make sure we understand how prow works with individual file permissions.

If I understand prow's function correctly, it looks like this PR would allow sig-cluster-lifecycle-kubeadm-approvers to bypass the sig-docs review cycle for a specific set of kubeadm docs. That's less than ideal.

A quick search shows that there are folder-specific OWNERS files in k/website that specify approver permissions in limited circumstances—specifically, for case studies and the blog. For the most part, though, it looks like files with individual approver permissions are legacy content in Chinese translation.

I've added this to the agenda for SIG Docs' next weekly meeting on 6/5 at 10:30am Pacific. We'd love it if you joined us for the discussion!

  1. Absent other concerns, there are merge conflicts to resolve.

Given #2 ☝️ it may make sense to hold off on resolving them.

/hold

EDIT: I want to be clear that I assume only 💯 awesome intent here. This PR looks like good housekeeping with approver: entries that were already present in the files, and it's excellent K8s citizenship of you to keep the docs clean and up to date. I just want to make sure we're not inadvertently introducing a bypass by preserving legacy language from when "approver" wasn't a magic prow keyword.

@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 May 31, 2018
@zacharysarah
Copy link
Contributor

/cc @mistyhacks

@mdlinville
Copy link
Contributor

@Bradamant3 told me just yesterday that nothing in /reference/ should ever get modified outside the scope of regenerating ref docs for a release. This is also full of conflicts. I'm too new to understand the implications of all this.

@steveperry-53
Copy link
Contributor

/hold

@steveperry-53
Copy link
Contributor

This needs some discussion.
Here's my view (and I thought this is what SIG Docs had agreed on): Individual files don't list approvers. Individual files only list reviewers. The only approvers are the people in sig-docs-maintainers.

@zacharysarah
Copy link
Contributor

@luxas 👋 Thanks for your patience with this PR. It looks like this PR inadvertently shortcuts the SIG Docs workflow for k/website by adding approvers: to title blocks. In order to avoid that, here's how we'd like to proceed.

Please update the title blocks for individual files to change approvers: into reviewers: and consolidate them. For example, this file's front matter should look like:

---
reviewers:
- sig-cluster-lifecycle-kubeadm-approvers
- sig-cluster-lifecycle-kubeadm-reviewers
title: Implementation details
content_template: templates/concept
weight: 100
---

Also, thanks for updating OWNERS_ALIASES and assigning reviews by alias! 🙇 Because we're requiring reviewers rather than approvers for individual files, it may change your strategy for organizing SIG Cluster Lifecycle members in OWNERS_ALIASES. (It's also fine to leave it as is; this PR needs no further changes to OWNERS_ALIASES to merge.) If you decide to change OWNERS_ALIASES, please make sure to update individual file blocks accordingly.

cc @steveperry-53

@neolit123
Copy link
Member

@zacharysarah

@luxas is on military service.
i can take care of this, but i don't see the need to update individual files.

@steveperry-53 yesterday confirmed that a single OWNERS file can be placed in a folder and it would apply to all files in folder - e.g. looking at content/en/docs/reference/setup-tools/kubeadm/

we are restructuring the kubeadm (cc @Bradamant3 , @timothysc ), which has a paralel timeline here.

my proposal for proceeding with this is the following:

  1. close this PR and i create a new PR that only adds:
- sig-cluster-lifecycle-kubeadm-approvers
- sig-cluster-lifecycle-kubeadm-reviewers

in the OWNERS_ALIASES file
2) proceed with the restructure of the kubeadm docs (this has already started)
3) once the docs are in place, remove the list of reviewers from individual kubeadm related docs.
(this can also happen in parallel with 2)
4) place individual OWNERS files in the appropriate kubeadm folders and use the new aliases from OWNER_ALIASES.

WDYT?

FYI: i'm in the #sig-docs channel if you want us to chat about this.

@neolit123
Copy link
Member

i guess i forgot to comment on PR approval in general.

i don't know about others, but i'm personally fine with sig-docs being the only approvers on k/website PRs as long as PRs get the appropriate tech reviews by pinging a sig. :)

@zacharysarah
Copy link
Contributor

@neolit123 👋

my proposal for proceeding with this is the following:

That works for me. 💯 Closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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.