tide plugin should ignore Reviewable and possibly other status checks #7140
Comments
|
@spxtr will fight you on this one. the idea is that all checks should be green before merging otherwise they aren't checks.
We don't want to override just any check though, many repos also use other CI
If it's useful to see the status, isn't it also useful for that status to eventually be green? That said I've expected that tide would eventually grow this feature. |
BenTheElder
added
the
kind/feature
|
reviewable is gone as of this morning ( cc @thockin ) per traffic on #sig-contribex channel there were just too many notifications getting generated. |
|
Heh, @BenTheElder knows me too well.
Fortunately it's a pretty simple feature to add. We already enumerate statuses on all PRs just to update the tide status. |
|
@dims reviewable is gone from kubernetes, but not kubeflow. |
|
ah thanks @BenTheElder ! |
As far as I can tell, reviewable requires reviewer to mark a comment thread as satisfied or non-blocking in order for the status check to be green. In practice, reviewers often just do "/lgtm" without marking each individual comment thread closed. |
|
Another example of a status check that caused problems for us was coveralls.io failing because of coverage decreases. I think it would be useful to be able to override failed status checks either as the result of /approve or with a specific command like /override. I thought "/skip" might do this but it didn't seem to. |
|
/skip clears the statuses to "skipped" / green. We cannot skip statuses we
didn't create.
…On Tue, Mar 6, 2018, 06:11 Jeremy Lewi ***@***.***> wrote:
Another example of a status check that caused problems for us was
coveralls.io failing because of coverage decreases.
I think it would be useful to be able to override failed status checks
either as the result of /approve or with a specific command like /override.
I thought "/skip" might do this but it didn't seem to.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7140 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4BqxQ_8834N1Chi5eZVQjomJdZgInQks5tbpkZgaJpZM4SdxqY>
.
|
|
Yeah, skip only cleans up optional statuses created by prow
On Tue, Mar 6, 2018 at 7:07 PM, Benjamin Elder <notifications@github.com>
wrote:
… /skip clears the statuses to "skipped" / green. We cannot skip statuses we
didn't create.
On Tue, Mar 6, 2018, 06:11 Jeremy Lewi ***@***.***> wrote:
> Another example of a status check that caused problems for us was
> coveralls.io failing because of coverage decreases.
>
> I think it would be useful to be able to override failed status checks
> either as the result of /approve or with a specific command like
/override.
> I thought "/skip" might do this but it didn't seem to.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/kubernetes/test-infra/issues/
7140#issuecomment-370793243>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA4BqxQ_
8834N1Chi5eZVQjomJdZgInQks5tbpkZgaJpZM4SdxqY>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7140 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADuFfzmNqKsllG-qV__20yf8Xx88KL0Bks5tbtBdgaJpZM4SdxqY>
.
|
|
For istio, users tend to approve and wait for the test pass, so I am not sure that I want /approve to overwrite anything. I think in those case, it is OK for admin to manually merge a PR as this should be an exception. However the required check is something istio uses a lot. When we add new jobs, we like to have them stable for quite sometime before making them required, and we also want people to see that this check is coming and that they should look at the status, but we don't want to impact the flow until it is fully stable. We also want to be able to disable a test quickly when something is going really wrong. There are as many github usage as org so it is fair for people to disagree. |
|
- /approve doesn't overwrite anything, `/skip` overrides *the github
statuses owned by prow that are not required to run against that PR*, the
job result / data is still in GCS etc.
- you can make sure a job is stable without displaying it on github with
`skip_report: true`, we do this all the time... you can check the stability
of the job on: deck, gubernator, testgrid
…On Wed, Mar 21, 2018 at 5:07 PM Sebastien Vas ***@***.***> wrote:
For istio, users tend to approve and wait for the test pass, so I am not
sure that I want /approve to overwrite anything. I think in those case, it
is OK for admin to manually merge a PR as this should be an exception.
However the required check is something istio uses a lot. When we add new
jobs, we like to have them stable for quite sometime before making them
required, and we also want people to see that this check is coming and that
they should look at the status, but we don't want to impact the flow until
it is fully stable. We also want to be able to disable a test quickly when
something is going really wrong. There are as many github usage as org so
it is fair for people to disagree.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7140 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq4Y9b_Np-sYVlhklVxtHXwgrlQjGks5tgutCgaJpZM4SdxqY>
.
|
|
Yes but I want the job to report while it is stabilizing. Also we don't only use Prow. |
I'm not sure we generally want to suggest this practice -- with large organizations, broadcasting to N developers working on PRs that job Z is a canary and should be ignored is very confusing. The point of reporting to GitHub is to give a developer feedback on their changes -- if the job is not stable, it's not giving any dev useful signal on their code. That being said, it seems like we have lazy consensus here that adding a means by which |
|
The fact that github has the required check options suggests this is a valid use case, and a use case that people want to use. If all required checks are passing, github will show it is safe to merge, otherwise merge will be block. if we don't support required check, we have inconsistent messaging. |
|
I think we're talking about two entirely separate issues -- I meant to say that exposing a job that is not stable to developers through a status on their PR is a bad idea for your organization. From a correctness for |
|
I agree. Istio is far from being perfect and does not have the maturity that k8s has. We don't even have a 1.0 yet. It is possible that we will not use this feature in 6 months. As you know behavioral changes take time and patience. I am working on a prototype code to get the required test working and the possibility to add all the other features, but I'll let those be implemented by people that need them. |
That said, I do think we need a way to make checks optional. I don't think |
|
Would it be possible to have an explicit command to tell tide to ignore failed status checks and merge the PR? Since filing this request I've encountered this issue with a number of different status checks reviewable, coveralls, Google CLA status check (and potentially many others). For some of these, like the Google CLA check its easy to get a status check which will never pass and not due to a problem with the PR. In which case manual merge/override is the only option. Configuring prow to treat that status check as optional probably isn't the right thing to do either. An explicit prow command to allow approvers to dismiss failed status checks is much better than manual merging IMO because a manual merge requires write permissions on the repo. One of the many reasons we (Kubeflow) love prow is that we can pretty much remove the need for anyone but the ci-bot to have permissions on the repo. |
|
How about a command trusted people can use to override a failing context? Like |
|
We will need to require |
|
/assign |
|
@stevekuznetsov seems perfectly reasonable to me. |
|
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
k8s-ci-robot
added
the
lifecycle/stale
|
/remove-lifecycle stale.
…On Tue, Jun 26, 2018, 12:34 fejta-bot ***@***.***> wrote:
Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually
close.
If this issue is safe to close now please do so with /close.
Send feedback to sig-testing, kubernetes/test-infra and/or fejta
<https://github.com/fejta>.
/lifecycle stale
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7140 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4Bq5cCjJcydCamjAygRzII_s503uoiks5uAozFgaJpZM4SdxqY>
.
|
|
This should now be possible. See the |
|
/close |
I think it would be useful if it was possible to configure tide to ignore certain status checks when deciding whether to merge or not.
I use reviewable for large PRs. With reviewable its easy for reviewers to forget to acknowledge/close all comments so that the status check for reviewable will pass.
My expectation is that if someone /lgtm and /approve this should override the reviewable status check.
Its possible to disable the reviewable status check; but I think it can be useful for people to see the status of the reviewable check in the GitHub issue.
The text was updated successfully, but these errors were encountered: