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

Translate 2019-03-07-raw-block-volume-support-to-beta.md #23060

Merged
merged 18 commits into from
Aug 18, 2020

Conversation

yixin21
Copy link
Contributor

@yixin21 yixin21 commented Aug 10, 2020

zh-trans 2019-03-07-raw-block-volume-support-to-beta.md

ref:k8smeetup/website-tasks#3218

zh-trans 2019-03-07-raw-block-volume-support-to-beta.md
@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 Aug 10, 2020
@k8s-ci-robot k8s-ci-robot added language/zh Issues or PRs related to Chinese language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Aug 10, 2020
@yixin21
Copy link
Contributor Author

yixin21 commented Aug 10, 2020

/assign @chenrui333

@netlify
Copy link

netlify bot commented Aug 10, 2020

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

Built with commit 86ad15e

https://deploy-preview-23060--kubernetes-io-master-staging.netlify.app

Copy link

@markthink markthink 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2020
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2020
yixin21 and others added 14 commits August 18, 2020 09:13
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
yixin21 and others added 2 commits August 18, 2020 09:26
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
…beta.md

Co-authored-by: Qiming Teng <tengqim@cn.ibm.com>
@tengqm
Copy link
Contributor

tengqm commented Aug 18, 2020

/lgtm
/approve
/tide-merge-method squash

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markthink, tengqm

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 Aug 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 79a2d03 into kubernetes:master Aug 18, 2020
@idealhack
Copy link
Member

idealhack commented Aug 18, 2020

/tide-merge-method squash

@tengqm it seems this command does not exist , you need to add the label manually.

@tengqm
Copy link
Contributor

tengqm commented Aug 18, 2020

@idalhack I didn't get an error on that.

@idealhack
Copy link
Member

yeah, there will be no errors because the bot treated that as a normal line instead of a command, but the result is no labels applied, and tide merged this PR using merge method, instead of your preferred squashing method, you can see all the commits on the master branch.

@tengqm
Copy link
Contributor

tengqm commented Aug 18, 2020

@idealhack Just checked the git log, you are right.
I'm afraid we need to revert this and merge it again.
18 commits for this is terrible.

@tengqm
Copy link
Contributor

tengqm commented Aug 18, 2020

I'm confused ... how can we fix this problem? The author was lazy when revising the PR. He/she was probably doing this revision in browser rather than from his/her git clone. If we revert this, we will create a new commit.

@idealhack
Copy link
Member

idealhack commented Aug 18, 2020

I think it's OK to let it be now, as we probably don't want to force push to master. If we revert the change, the 18 commits won't go anywhere.

Could you check if you are able to apply labels to PRs manually? You'll need write access to the repo for that.

If you have the ability, please use it from now on, otherwise, please ask future PR authors to rebase locally and force push before merge.

@idealhack
Copy link
Member

I'd also make a note to other approvers in SIG docs, as I see this is not a single case. It would be great if you can make a note in the next SIG docs meeting.

@idealhack
Copy link
Member

Never mind, I found the right command, it's /label tide/merge-method-squash, please use it from now on and make a note to the SIG.

@jimangel
Copy link
Member

@tengqm @idealhack is correct, you can use the label /label tide/merge-method-squash in the future (my reference cheat: https://kubernetes.io/docs/contribute/participate/pr-wranglers/#helpful-prow-commands-for-wranglers).

While it is unfortunate that this merged without a squash, I think it's better to leave it vs. creating additional commits or force rewriting history.

CC'ing tech leads for awareness (and comments if they disagree 😄 ): @kbhawkey @onlydole @sftim

@kbhawkey
Copy link
Contributor

kbhawkey commented Aug 18, 2020

@tengqm @idealhack is correct, you can use the label /label tide/merge-method-squash in the future (my reference cheat: https://kubernetes.io/docs/contribute/participate/pr-wranglers/#helpful-prow-commands-for-wranglers).

While it is unfortunate that this merged without a squash, I think it's better to leave it vs. creating additional commits or force rewriting history.

CC'ing tech leads for awareness (and comments if they disagree 😄 ): @kbhawkey @onlydole @sftim

Hi. Yes, at this point it seems like the best option is to leave the multiple commits (as long as the Blog page builds 😃 ).

When I review a PR, I compare the number of files changed against the number of commits.

If the number of commits is greater than the number of files changed, I start to look for why the commits are needed.
I hold off of approving any PRs where the ratio is higher for the number commits.
Ideally, I prefer to have one commit per PR (or at the most 2 commts).
Perhaps we could use an automatic warning label from the bot
that alerts reviewers about the number of commits (some percentage of num commits to num files changed)?

@sftim
Copy link
Contributor

sftim commented Aug 18, 2020

I'd leave it, yep.

Also even if you set the correct label, things can race. Better to set that label in one comment and then add a second comment to LGTM or approve, after the label is added.

@tengqm
Copy link
Contributor

tengqm commented Aug 18, 2020

Thanks for the feedbacks. I apologize for this incident. I was trying to save everyone's time by leveraging the automation. Obviously, the imagined prow command didn't work. It was not that I failed to notice that there are 18 commits. I did a bad thing by issuing a strange command to the bot and I hoped that the bot would say "command not recognized, try something else...". However, I got no error. Anyway, I learned something from this.

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. language/zh Issues or PRs related to Chinese language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. 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.

9 participants