Allow /approve to be misspelled as /approved#23335
Allow /approve to be misspelled as /approved#23335danwinship wants to merge 1 commit intokubernetes:masterfrom
Conversation
|
@danwinship this one is another one openshift/origin#26344 |
alvaroaleman
left a comment
There was a problem hiding this comment.
/hold
I like it, @cblecker @chaodaiG @stevekuznetsov WDYT?
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, danwinship The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
chaodaiG
left a comment
There was a problem hiding this comment.
LGTM as well, left a nit
There are two different places this could be mentioned:
test-infra/prow/plugins/approve/approve.go
Line 132 in ede436e
OR
https://github.com/kubernetes/test-infra/blob/master/prow/ANNOUNCEMENTS.md
prow/plugins/approve/approve.go
Outdated
| PluginName = "approve" | ||
|
|
||
| approveCommand = "APPROVE" | ||
| wrongApproveCommand = "APPROVED" |
There was a problem hiding this comment.
nit: wrongApproveCommand implies error to me, how about change it to misspelledApproveCommand?
alternatively, it might also take a slice like alternativeApproveCommands, or just combine with APPROVE to be underl approveCommands
There was a problem hiding this comment.
ok, I went with alternateApproveCommand. Doing a slice would have required a bunch more rewriting...
|
New changes are detected. LGTM label has been removed. |
|
LGTM, please fix GOFMT error and I'll approve |
|
From a ContribEx standpoint, we have always shied away from aliases for core commands (especially undocumented ones). The reason for this, is it makes it difficult for new contributors to know and understand what the flow looks like and what they should be doing. I know there's a desire to be smart here and try and "do the right thing" based on intent, but these have consistently shown to be problematic in the larger picture. /hold |
Do we document such issues around command aliases / decisions about moving away from aliases? With data (examples etc) and reasoning. |
|
Maybe if aliases won't be supported, we can get more data around this later and if too many people use |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
|
@danwinship: PR needs rebase. DetailsInstructions 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. |
|
@danwinship: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
|
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
|
@k8s-triage-robot: Closed this PR. DetailsIn response to this:
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. |
Tim did this here a while back and then Clayton just did it twice here and here, and those are just ones I know about... (It doesn't seem to be possible to search PRs for the string "
/approved"; github just ignores the slash.)Anyway, a modest proposal.
(This intentionally does not change any of the documentation, though maybe it ought to be mentioned somewhere?)