tide: status is opaque

[stevekuznetsov]

Previously, the merge queue would leave a comment like "missing text context" or "missing LGTM label" whereas now we have "Not in tide pool." That is hard to understand and users are having issues taking the appropriate actions just from that message.

/kind bug /area prow /cc @kargakis @cjwagner

[stevekuznetsov]

/cc @spxtr

[spiffxp]

ref: https://github.com/kubernetes/test-infra/pull/6010#discussion_r157597197

[stevekuznetsov]

Also FWIW tide as a context name itself is pretty opaque -- we had Submit Queue before which at least helped users understand what they were looking at. We should maybe use something more user friendly or at the very least prefix with the common continuous-integration/(prow/)?.

[spxtr]

Yep, this is totally something I plan on doing next. /assign

[0xmichalis]

We should maybe use something more user friendly or at the very least prefix with the common continuous-integration/(prow/)

+1

[mattfarina]

Note, the tide status also makes it difficult to know the state of the PR on the PR listing pages. For example,

Because tide isn't reporting status the PR listing page shows that PRs are still waiting on report. Even if all the other CI runs have passed. At a glance it's not possible to see which PRs are passing all CI runs on the listing pages.

[BenTheElder]

@mattfarina I don't think this part is different from the submit-queue behavior FWIW. Tide will report green once a PR is eligible for merge. @spxtr thoughts on the tide status just always being green? :man_shrugging:

[mattfarina]

@BenTheElder That could be. Guess there isn't a way to have a dashboard of all the things passing CI for someone to go review.

[mattfarina]

This is more of a UX issue so maybe it's worth discussing in contribx. I just ran across this...

[stevekuznetsov]

@mattfarina I think the point of this issue is to address exactly the issues you're reporting -- the intent here is that the status will be reporting why the PR is not in the tide pool yet. As far as "dashboard of all the things passing CI", why is the status listing at the bottom of the PR an appropriate place for a user to look for CI statuses?

[mattfarina]

@stevekuznetsov the CI status at the bottom of a PR isn't the place to get the overview. But, the aggregate of that status is what populates PR listings like the one at https://github.com/kubernetes/test-infra/pulls. It's about how it affects the aggregate displayed elsewhere.

[spxtr]

@spxtr thoughts on the tide status just always being green? :man_shrugging:

I think that's misleading. Green tide status -> all is good. Yellow -> some action needs to be taken.

We should maybe use something more user friendly or at the very least prefix with the common continuous-integration/(prow/)?.

I'd be fine with that, although the more we put there the less we can put in the description.

[stevekuznetsov]

I'd be fine with that, although the more we put there the less we can put in the description.

Agreed. We should just use a string that's long enough so nobody is immediately asking "wait, what on earth is tide?"

[cjwagner]

I want to make some progress on this before enabling Tide on k/k. I think for now, changing the statuses to be one of tide - In merge pool. or tide - Not in merge pool. is sufficient to clarify that Tide is a merge queue like the Submit Queue status context that it is replacing. When we migrate the SQ status context to the tide context using the migratestatus tool the Submit Queue context description will be updated to indicated that it has been replaced by the tide context which should also help reduce confusion.

[heckj]

SIG-DOCS has been hitting a little of this learning - as a suggestion, enabling some UX so that "how it works" is more easily found. Right now the link from "not in the tide pool" takes you to a fairly inscrutable page, and if there was a "HOW THIS WORKS" kind of link within there to https://github.com/kubernetes/community/blob/master/contributors/devel/owners.md there would be a path to getting the information.

As a straw man, let me suggest changing the message "not in the tide pool" to "tags not yet set for merge approval", which I think indicates a little more clearly what may be needed in terms of actions from submitters, reviewers, and approvers.

[stevekuznetsov]

@cjwagner is it not going to be possible to thread through the specific items that are keeping a PR from being in the tide pool?

[cjwagner]

@stevekuznetsov That is what we will ultimately do. I'm trying to make a simpler fix before deploying to k/k though. Updating tide to explain why a PR isn't in the pool via the status context will take more work (what if a repo is listed in multiple queries? which query should we try to satisfy?).

@heckj The Merge Requirements section on that page expands to show the merge requirements for each repo visually. We plan to eventually improve the status context details link to go to a customized version of this page that only shows info relevant to your PR. The documentation you linked isn't actually related to Tide, but I I'm going to add some documentation for Tide and will make sure that it is reachable from the Tide dashboard.

[0xmichalis]

what if a repo is listed in multiple queries

We can validate this during config load.

[cjwagner]

We can validate this during config load.

Having multiple queries include the same repo is a valid config though. Specifically it is necessary to require at least one of a set of requirements such as labels. For example I might use these queries to get any PR that has some sort of positive CLA label:

repo:kubernetes/test-infra label:"cncf-cla: yes"

repo:kubernetes/test-infra label:"cncf-cla: human approved"

Tide unions query results before partitioning the results into subpools specifically to allow for this behavior.

[BenTheElder]

As a straw man, let me suggest changing the message "not in the tide pool" to "tags not yet set for merge approval", which I think indicates a little more clearly what may be needed in terms of actions from submitters, reviewers, and approvers.

Tags not yet set for merge approval doesn't cover failing / missing tests?

@qhuynh96 is starting on a user dashboard with github login to allow users to see their PR status on prow.k8s.io someday, we can only pack so much detail into the status context on the PR, the strings are very quickly truncated.

[mattfarina]

@BenTheElder GitHub has a dashboard where I can view my PRs for a repo. I can see the CI status and the tags on the PR. At a glance I can see if CI is passing and any status passed in labels. If our use of CI status just reflected CI this could tell people if all the tests are passing and if it's been approved/reviewed (via the labels). Is there some other status a dashboard needs to convey?

[stevekuznetsov]

@cjwagner I'm not sure deploying to k/k and exposing that many devs to the (poor) UX of the current status (or even the proposed "Not in merge pool") when it's a clear regression from the Submit Queue behavior is a good idea.

[BenTheElder]

Mergeability also depends on having or not having labels and or native github review depending on the repo's configuration. The intention is to provide a similar dashboard that understands our merge process. GitHub can't really do that in their dashboard. Passing tests != ready to merge.

On Fri, Jan 26, 2018, 06:35 Matt Farina notifications@github.com wrote:

@BenTheElder https://github.com/bentheelder GitHub has a dashboard where I can view my PRs for a repo. I can see the CI status and the tags on the PR. At a glance I can see if CI is passing and any status passed in labels. If our use of CI status just reflected CI this could tell people if all the tests are passing and if it's been approved/reviewed (via the labels). Is there some other status a dashboard needs to convey?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-360800520, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bqxt990dS1cajShOBTLDtvzc96YN2ks5tOeKbgaJpZM4RTLGa .

[BenTheElder]

The submit-queue also sets a yellow context until mergeability is met.

On Fri, Jan 26, 2018, 06:35 Matt Farina notifications@github.com wrote:

@BenTheElder https://github.com/bentheelder GitHub has a dashboard where I can view my PRs for a repo. I can see the CI status and the tags on the PR. At a glance I can see if CI is passing and any status passed in labels. If our use of CI status just reflected CI this could tell people if all the tests are passing and if it's been approved/reviewed (via the labels). Is there some other status a dashboard needs to convey?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-360800520, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bqxt990dS1cajShOBTLDtvzc96YN2ks5tOeKbgaJpZM4RTLGa .

[0xmichalis]

Until the dashboard solution is proven, we need to ensure the context does not regress as most of our users depend on it today.

On Fri, Jan 26, 2018 at 6:27 PM, Benjamin Elder notifications@github.com wrote:

The submit-queue also sets a yellow context until mergeability is met.

On Fri, Jan 26, 2018, 06:35 Matt Farina notifications@github.com wrote:

@BenTheElder https://github.com/bentheelder GitHub has a dashboard where I can view my PRs for a repo. I can see the CI status and the tags on the PR. At a glance I can see if CI is passing and any status passed in labels. If our use of CI status just reflected CI this could tell people if all the tests are passing and if it's been approved/reviewed (via the labels). Is there some other status a dashboard needs to convey?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/ 6145#issuecomment-360800520, or mute the thread https://github.com/notifications/unsubscribe-auth/ AA4Bqxt990dS1cajShOBTLDtvzc96YN2ks5tOeKbgaJpZM4RTLGa .

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-360849498, or mute the thread https://github.com/notifications/unsubscribe-auth/ADuFf_w92UIEzCr9hYOfUossn3aFO-fHks5tOgrxgaJpZM4RTLGa .

[mattfarina]

@BenTheElder please forgive me if this has already been discussed but there's an impact to contributor experience.

The status API on GitHub is almost always used to report testing or CLA status. I went looking around and could not find a non-k8s repo that used it for cases other than this. For those not working with Kubernetes they see the status as tests passed, failed, or are pending. This impacts how someone uses it. Are pull requests passing tests so they are ready to be reviewed? That's how these statuses are often used.

On Kubernetes it's different. It's, a question of... are these pull requests ready to be merged? The status isn't about passing tests and CLAs but something else. For new contributors this is a difference they need to somehow learn (where?). For contributors who spend much of their time on other projects it's a context switch which takes on a cognitive load. It also means their typical flow of looking at PR lists to see what's ready to review doesn't work so they need an alternative workflow (where is that at?).

I'm not even sure where I'd go to see the PRs that are passing all tests and ready to be reviewed compared to those that are pending (because some test runs take a long time).

This change to the contributor experience apart from expectations (rather than being layered on) and the change in who those rolled up statuses are useful for it what I'm getting at.

Maybe this experience change has been discussed elsewhere and I'm rehashing old news. I just want to make sure that the impact to contributor experience is intentional and handled well.

[BenTheElder]

I don't think it has. But what tide is doing is continuing the submit-queue behavior for the moment, the only difference is in the textual description / status name.

Outside of kubernetes this doesn't seem common because merge automation doesn't seem particularly common (in open source at least). So those who aren't used to this extra status may not be used to what it is there for to begin with. That really means we should try to surface it.

For finding PRs we have k8s-gubernator.appspot.com/pr and Quang is building a new tool.

Also FWIW in kubernetes core at least since tests are often quite flaky and PR authors need help it's not a great experience to not touch an incoming PR until everything is 100% green anyhow.

I definitely get your point about the status being yellow on GitHub hiding the test results in the overall status but I don't think we should deviate far from the submit-queue behavior here until after the transition and after a discussion involving more than the few of us on this issue.

On Mon, Jan 29, 2018, 08:48 Matt Farina notifications@github.com wrote:

@BenTheElder https://github.com/bentheelder please forgive me if this has already been discussed but there's an impact to contributor experience.

The status API on GitHub is almost always used to report testing or CLA status. I went looking around and could not find a non-k8s repo that used it for cases other than this. For those not working with Kubernetes they see the status as tests passed, failed, or are pending. This impacts how someone uses it. Are pull requests passing tests so they are ready to be reviewed? That's how these statuses are often used.

On Kubernetes it's different. It's, a question of... are these pull requests ready to be merged? The status isn't about passing tests and CLAs but something else. For new contributors this is a difference they need to somehow learn (where?). For contributors who spend much of their time on other projects it's a context switch which takes on a cognitive load. It also means their typical flow of looking at PR lists to see what's ready to review doesn't work so they need an alternative workflow (where is that at?).

I'm not even sure where I'd go to see the PRs that are passing all tests and ready to be reviewed compared to those that are pending (because some test runs take a long time).

This change to the contributor experience apart from expectations (rather than being layered on) and the change in who those rolled up statuses are useful for it what I'm getting at.

Maybe this experience change has been discussed elsewhere and I'm rehashing old news. I just want to make sure that the impact to contributor experience is intentional and handled well.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-361308655, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq1zx-5XOI0K5IaRHucsXFYf4Cv4Cks5tPfZugaJpZM4RTLGa .

[cjwagner]

I've discussed this issue and related issues with others offline in detail and thought I should share what I'm thinking and try to summarize the design discussions we've had about these topics. cc @BenTheElder @rmmh @stevekuznetsov @kargakis @fejta @mattfarina Commence wall of text:

Tide needs some presence on Github PR pages.

Users need to know if their PR is currently meeting merge requirements without going to another page. If their PR is not meeting merge requirements there should be some indication of what their next step should be to meet merge requirements. This could be achieved in three ways:

1. Maintaining a comment on the PR similar to the [APPROVAL NOTIFICATION] comments.
   • This would probably provide the best UX because the comment could contain as much content as needed. Additionally it would not make the PR's combined status appear as pending if only the Tide context is pending (see the "Maintaining a status context" section).
   • The API token cost could be much more expensive because we would have to list comments in order to find and update the correct one. If the status context is managed by Tide's sync loop instead of a separate plugin we can ask for the last 100 comments, but we may still have to list all comments if the comment we want is not present and this would happen every minute for every applicable PR.
2. Maintaining a section in the PR description
   • This is similar to having Tide maintain its own comment, but avoids the API token cost problem because the body of the PR is provided directly in webhooks and via the graphql API.
   • Having the bot significantly modify PR bodies seems like a bad practice?
3. Maintaining a status context:
   • This is what the Submit Queue currently does so this behavior would be consistent with the current process. This is a big plus, changing workflow/process is confusing and can be harmful to developer velocity.
   • If we use a status context to display a PR's state with respect to merge requirements Github will include that context as part of the combined status. Some people find this confusing because it can seem like the PR is still running tests even if it isn't when viewed from a Github page that shows a list of PRs. However, this is something that some k8s contributors actually like because it shows if the PR is being considered for merge. For new k8s contributors who are already familiar with Github it is also a clear signal that a PR needs attention.

Separate plugin to maintain status context

Pros:

• Tide is decoupled from status context logic.
• We can react to events instead of updating the context while syncing. This is more responsive.
• Could reduce token usage unless Tide is running on very active repositories.

Cons:

• Potential for races caused by near-simultaneous events.
• Potential for status to disagree with Tide if something goes wrong with one component.
• This could potentially increase API token usage instead of reducing it. Responding to every event means that we could process PRs more frequently. Furthermore, unlike the graphql queries used by Tide, the webhook payloads a plugin receives would not contain complete info about each PR and the plugin would frequently need to make additional queries to github to get info like the labels and reviews on the PR. I think that the plugin would be more efficient if Tide is encountering fewer than ~12 changed PRs per sync loop based on the following token usage estimates: (changes are only considered by the plugin if they affect a merge requirement)

  tide cost per minute > plugin cost per minute
  open/100 + changed_per_min > changed_per_minute * cost_per_pr
  open/100 > changed_per_minute * (cost - 1)
  open/(100 * cost - 100) > changed_per_minute
  7.5 > changed_per_minute (assuming open=1500 and cost=3, the cost to list labels+reviews and set status)
  15 > changed_per_minute (assuming open=1500 and cost=2, the cost to list labels and set status which is more accurate for most k8s repos)

  On average I think the plugin would be better, but using Tide's sync loop wouldn't be as likely to exhaust API tokens during high load. That being said, tide would always use ~900 API tokens an hour just to list all open PRs every minute without any changes to PRs. I'm not sure which is the best option.

Determining what a PR needs to do to meet merge requirements

Current state and problem

Currently Tide allows a repo to appear in multiple Tide queries. This is considered valid config and is necessary to enforce any merge requirements in which we require PRs to have at least one of a set of requirements. In other words, supporting this is necessary for Tide to be able to handle requirement disjunctions (the OR logical operator). For example I might use these queries to require that all PRs have some sort of positive CLA label without requiring a specific one:

queries:
- repos:
    - kubernetes/test-infra
    labels:
    - "cncf-cla: yes"
- repos:
    - kubernetes/test-infra
    labels:
    - "cncf-cla: human approved"

Tide unions the sets of PRs returned by each query before partitioning the PRs into subpools specifically to allow for this behavior.

This makes it difficult to automatically describe why a PR is not meeting merge requirments because there may be multiple disparate requirement sets that the PR could meet so the next action the user should take could be unclear.

Proposition

I suggest that we stop supporting this behavior for the time being given that we don't currently require it and given that the use cases we may eventually encounter can be satisfied in different ways.

The main use case for this behavior is requiring at least one label from a set of labels. This behavior can be provided in a few other ways:

• We could make this a first class feature of Tide queries by adding a 'label-set' section to the Tide query config that allows queries to specify sets of labels for which at least one must be present. Tide could then automatically split the query into multiple queries and union the results. This would make determining missing merge requirements for a PR easy as we could enforce 1 query per repo.
• We could do some complicated logic to determine and display the user's options for matching different queries.
• The best option is using a prow plugin to manage the label set and provide an indicator label that blocks or allows merge. We already do this with a number of plugins. An example of this is the needs-sig label which is applied if a PR does not have at least one sig/* label. The do-no-merge/release-note-needed label is also managed and used in this way. This pattern is favorable because it doesn't require Tide to make additional search queries every minute in order to union the results. Instead the plugins react once to the label events on the PR.

Until we need to support requirement disjunctions we can just add a presubmit test that ensures no repos appear in multiple queries.

Action items

• [x] Decide how we want to give Tide a presences on PR pages. I think we should continue using a status context for now since it is already implemented in some form and because this is the current Submit Queue behavior.
• [ ] Decide if we want to use a Prow plugin to maintain Tide info on PR pages instead of using Tide itself.

Assuming my proposition to enforce "one query per repo" is reasonable:

• [ ] Start enforcing "one query per repo" with a presubmit test.
• [ ] Write code to generate a list of merge requirements that a PR is not meeting.

[BenTheElder]

Users need to know if their PR is currently meeting merge requirements without going to another page.

Do they? I think maybe they just need a convenient way to check. So far we've used a status on the page and ported this over, but if we're not going to behave the same as the submit-queue style then one click away to more helpful results is probably acceptable.

We could create an initial comment with a link to a status page for the PR or something similar. We can provider richer feedback outside the constraints of status labels if we go this route.

We could do some complicated logic to determine and display the user's options for matching different queries.

It should be pretty simple to determine the least-different merge requirements from the PR. I've already discussed doing so on the proposed deck user dashboard with @qhuynh96. We can just EG count the number of labels that are wrong versus the PR and sort the queries / merge-requirements by this.

The plan was to use the (logged-in) users' tokens for queries though, I'm not sure how tide would obtain this information on PRs that are not in the pool cheaply without becoming mungegithub 2.0 ...?

[cjwagner]

I definitely think linking to the user dashboard will provide a much more complete resource for assessing merge requirements for a PR, but I think that getting actionable information directly on the PR page is pretty valuable, especially because users are already used to this with the Submit Queue.

The plan was to use the (logged-in) users' tokens for queries though, I'm not sure how tide would obtain this information on PRs that are not in the pool cheaply without becoming mungegithub 2.0 ...?

An HTTP caching proxy layer in front of Prow's components would alleviate these problems and is something that we will eventually need anyways. Github API requests would be free if nothing had changed since the last request. This should also be very easy to set up. Given that just listing all open PRs will require about 15 tokens per minute (900 per hour) we might also want to give Tide its own bot account. It will end up using even more tokens than that setting statuses and listing file changes on PRs.

[stevekuznetsov]

All of the experience I have personally with systems that post and maintain comments on pull requests is negative. Consistently if the comment is edited and not re-created it drifts to the top of the event stream in the PR and is ignored by contributors. We used to have an edited comment that would let users know what their tests were doing and we got pinged so many times asking what the status of PR X was where the user just didn't scroll up to see the comment that had been updated. Similarly, full HTML is available to us in the comment so potentially we could find something that works but the current approval comment is a great example of very dense information presented in a comment that many users find hard to read or gloss over. I think a comment would be the least useful way to expose this information to the user and I think there is a strong argument for using the status context to provide a consistent UX with the Submit Queue.

Re: one click away ... one click is huge. We for ages had our jobs run as a suite in Jenkins and only the aggregate information of the suite pass/fail was reported to GitHub, users had to make on click to Jenkins to determine which of their test suites (think test-cmd, test-integration, test-e2e) failed. This one click for information was universally hated and the move to separate status contexts using mungegithub was praised heavily. I'd strongly suggest not putting clicks between the users and the information. I think it's reasonable if we still have a details link on the tide status context that brings you to some page in deck that perhaps has more in-depth information.

Regarding a plugin, I guess I would ask if we really need the status of the PR to be updated immediately and if that would actually be useful. I can see some issues if the plugin, responding to events, has the PR marked as ready to merge but the tide loop has not yet run and a user is confused to see that the PR is marked as ready but not in a batch merge or whatever. Any possibility, also, that the plugin and the tide loop will generate conflicting results on the state of a PR would be a headache. If we can update the status context from the tide loop my first guess is that this would be the simpler approach.

[0xmichalis]

All of the experience I have personally with systems that post and maintain comments on pull requests is negative.

True. As a Kubernetes developer, I was always annoyed by the fact that I get tons of emails from the bots. Hearing the same complain in Origin today. Related https://github.com/kubernetes/test-infra/issues/1723 https://github.com/kubernetes/test-infra/issues/4460

Re: one click away ... one click is huge.

My thoughts and experience as well. Trying to convince folks to adopt new flows is hard. Really. People are comfortable enough (most of the time, not always) with Github statuses. I know folks though that until a couple of months ago didn't even know that SQ had a dashboard because they never bothered clicking the Details button in Github.

[BenTheElder]

I'm not suggesting we place the click down in the status details though, instead in an opened-PR-bot-comment with better visbility.

GitHub statuses generally have poor UX for anything beyond "test foo failed" or "test foo passed". They're aggressively truncated and don't even show up to users that aren't logged in.

On Thu, Feb 1, 2018 at 8:44 AM, Michalis Kargakis notifications@github.com wrote:

All of the experience I have personally with systems that post and maintain comments on pull requests is negative.

True. As a Kubernetes developer, I was always annoyed by the fact that I get tons of emails from the bots. Hearing the same complain in Origin today. Related #1723 https://github.com/kubernetes/test-infra/issues/1723 #4460 https://github.com/kubernetes/test-infra/issues/4460

Re: one click away ... one click is huge.

My thoughts and experience as well. Trying to convince folks to adopt new flows is hard. Really. People are comfortable enough (most of the time, not always) with Github statuses. I know folks though that until a couple of months ago didn't even know that SQ had a dashboard because they never bothered clicking the Details button in Github.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-362326072, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq9EnSeFkz5GN3QgOjCReBSndgh6pks5tQenYgaJpZM4RTLGa .

[rmmh]

+1 for a status like "submit-queue/tide: tests not passing, missing lgtm and approved labels [details]" with an extra click for more details.

Do we care about users not logged into github?

[BenTheElder]

Do we care about users not logged into github?

probably not, they're still not particularly great UX but per discussion at sig-testing standup today I think we all agree it's best to try to mimic the submit-queue here since so many users already know this. We probably should link to a more detailed dashboard though since only telling users part of what is wrong stinks.

Thoughs from this morning with @stevekuznetsov @cjwagner @rmmh @krzyzacy:

• keep a submit-queue esque status listing at least one thing wrong with the PR
• use the details link to link to something more useful / detailed once that exists
• start listing all open PRs in tide to support this
  • if this becomes an API usage problem, split up write / read tokens and just make more bots

[cjwagner]

start listing all open PRs in tide to support this

Technically, Tide is already doing this now in order to set statuses on PRs not in the pool. It isn't yet an API usage problem, but could become one once it is enabled on k/k (+1000 open PRs). I think it is reasonable for us to require 2 bot accounts to handle GH automation given the scale of the kubernetes org.

[spxtr]

As @cjwagner said, we already list all open PRs. It's 100 PRs per page, although the graphql api has different rate-limiting logic than the rest api.

1000 open PRs means that we will consume a little over 1000 v3 tokens (one per PR) when we initially turn on the more complicated status contexts, as well as any time we change the context format. Rather than using two bots, I would prefer to improve our self-rate-limiting. It's really not a big deal if statuses are slow to update when we change the description format, and if we design it well then it will help limit the impact of bugs in the future.

+1 for a status like "submit-queue/tide: tests not passing, missing lgtm and approved labels [details]" with an extra click for more details.

I've mostly finished a PR that will do this. Hopefully this weekend I can get that out for review.

[cjwagner]

It's really not a big deal if statuses are slow to update

Agreed. We can throttle the number of tokens we use setting status contexts without affecting the main sync loop in a number of ways.

Another thing I just though of is that the search for all open PRs (needed to check and update status contexts) could query only for PRs that have changed since the last sync loop using the updated: search field: https://help.github.com/articles/searching-issues-and-pull-requests/#search-based-on-when-an-issue-or-pull-request-was-created-or-last-updated

That call will be responsible for the vast majority of the token usage so only checking recently updated PRs could reduce token usage a ton.

[mattfarina]

It appears this thread is making a bunch of assumptions about tide users and their needs. Are these detailed and verified anywhere?

Some things to consider:

• tide is used on the websites/docs, charts, community, and numerous repos in addition to k8s/k8s (when it gets there). This is wider use than 6 months ago when the functionality was primarily used for k8s/k8s and community (in a limited sense)
• Who needs to know when something passes testing or linting vs who needs to know when something is ready to merge? Of those uses who do we prioritize and put in a public space (PR listings) vs our own dashboards?
• The kubernetes org has 583 members which is about 1/3 the number of people on the k8s/k8s repo. And, there are a bunch of org members who don't contribute to k8s/k8s (the most complicated case). How do we prioritize regulars vs the long tail of contributors in this experience?
• The typical repo usage of the GitHub status API is to report on CIs or CLA/DCOs. This is the expectation most people who use GitHub have (are any other projects using it for something other than that?). For the long tail of contributors how does our change (the status says when it's ready to merge instead of tests passing) impact their ability to navigate contributing?

This is very much about contributor experience. How can we navigate that?

cc @parispittman

[stevekuznetsov]

For the long tail of contributors how does our change (the status says when it's ready to merge instead of tests passing) impact their ability to navigate contributing?

@mattfarina the point of this issue is to ensure there is no change from the UX that was present with the Submit Queue.

[cjwagner]

+1 for a status like "submit-queue/tide: tests not passing, missing lgtm and approved labels [details]" with an extra click for more details.

I've mostly finished a PR that will do this. Hopefully this weekend I can get that out for review.

@spxtr What is the status of this? I'm happy to help with this or take over if you are busy. It would be nice to get this working soon.

[stevekuznetsov]

@spxtr did we have a WIP PR or branch pushed up with the work you mentioned above?

[spxtr]

Sorry for the long delay, things have gotten interesting over here. I mostly likely won't be able to drive this one to completion, especially since you'll want to pay close attention as you deploy it.

https://github.com/spxtr/test-infra/commit/c2620fc8dcfb4412ae7493cdccf6d307fd0fc6e5

This works for labels and missing labels. It's so simple (and untested)! A manual dry run shows it wants to set statuses for about 600 PRs. See the commit message as well. Make sure that github doesn't complain if you try to set a long status description.

/unassign /assign @cjwagner

[BenTheElder]

Thanks Joe! 😄

On Fri, Feb 16, 2018 at 7:26 PM, Joe Finney notifications@github.com wrote:

Sorry for the long delay, things have gotten interesting over here. I mostly likely won't be able to drive this one to completion, especially since you'll want to pay close attention as you deploy it.

spxtr/test-infra@c2620fc https://github.com/spxtr/test-infra/commit/c2620fc8dcfb4412ae7493cdccf6d307fd0fc6e5

This works for labels and missing labels. It's so simple (and untested)! A manual dry run shows it wants to set statuses for about 600 PRs. See the commit message as well. Make sure that github doesn't complain if you try to set a long status description.

/unassign /assign @cjwagner https://github.com/cjwagner

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#issuecomment-366412597, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq3o4J9JNX61vJy98GTDniQ1U41pNks5tVkbjgaJpZM4RTLGa .

[BenTheElder]

BTW the PR status page is also almost ready for wider use and allows custom queries so it should be possible to find passing PRs on there soon. We may need to extend its feature set a little first.

On Tue, Mar 13, 2018, 07:52 k8s-ci-robot notifications@github.com wrote:

Closed #6145 https://github.com/kubernetes/test-infra/issues/6145 via

7197 https://github.com/kubernetes/test-infra/pull/7197.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kubernetes/test-infra/issues/6145#event-1518775725, or mute the thread https://github.com/notifications/unsubscribe-auth/AA4Bq3sdM6Uex4qJNHuYWAt5uMlGXrp4ks5td90bgaJpZM4RTLGa .

[stevekuznetsov]

:clinking_glasses: :tada: