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

PR Status now shows PR contexts along with Prow Jobs #8612

Merged
merged 8 commits into from Jul 19, 2018

Conversation

@qhuynh96
Copy link
Member

commented Jul 9, 2018

  • Fixes #7235
  • Screenshot:

screen shot 2018-07-18 at 5 19 25 pm

qhuynh96 added some commits Jul 7, 2018

@qhuynh96

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

/assign @BenTheElder
/assign @cjwagner
/assign @stevekuznetsov

@qhuynh96

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

/area prow

@stevekuznetsov
Copy link
Contributor

left a comment

Couple questions but mostly LGTM!

// PullRequestWithContext contains a pull request with its latest context.
type PullRequestWithContext struct {
Contexts []Context
PullRequest PullRequest

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Jul 9, 2018

Contributor

You could embed PullRequest here

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

Did you mean Contexts could be a field of PullRequest

This comment has been minimized.

Copy link
@qhuynh96
@@ -86,6 +83,14 @@ type Label struct {
Name githubql.String
}

// Context holds graphql response data for github contexts.
type Context struct {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Jul 9, 2018

Contributor

Why use the githubql types if we have to coerce into them anyway?

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

Fixed

@@ -90,117 +115,230 @@ func TestServeHTTPWithoutLogin(t *testing.T) {
if err := yaml.Unmarshal(body, &dataReturned); err != nil {
t.Errorf("Error with unmarshaling response: %v", err)
}
if !reflect.DeepEqual(dataReturned, mockData) {
if !equality.Semantic.DeepEqual(dataReturned, mockData) {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Jul 9, 2018

Contributor

equality.Semantic is useful for k8s types and adds a level of semantic meaning to the checks, which isn't (?) appropriate for these types. was reflect not working?

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

I have some problems with reflect before that I could not remember what they were. Maybe I was wrong, I will use reflect instead

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

Fixed

});
}
}
if (builds) {

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Jul 9, 2018

Contributor

If we're overriding something here it would be awesome if we showed a warning icon and had some way for a user to discover that something was wrong... otherwise having inconsistent state is basically invisible to users and that is very frustrating

/cc @cjwagner @BenTheElder

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 9, 2018

Member

hmm yeah, I think if we have a github context we should prefer that and only show build statuses where either:

  • there's no github context for some reason (??)
  • Possibly when the github context is knowably older than the prowjob state, EG github is still showing yellow but we know the job finished and is passed/failed (I don't think we can trivially detect this right now, but maybe in the future). Even then the inconsistency probably makes this not worth it.

I'd also say having some way to detect the source of the state would be helpful, maybe we can mark them with the github / prow logos?
Or in the future the avatars of the account posting the status + some other icon when it's just a prowjob state.

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

@stevekuznetsov

a warning icon and had some way for a user to discover that something was wrong

and

having inconsistent state is basically invisible to users

Can you explain more about those points?
@BenTheElder
I see. For your second point, our deck couldn't update data in real-time which means that users must refresh the page to see fresh info; therefore, by the time they refresh, Github contexts may already up to date with Prow Jobs. But I still dont get why we prefer Github contextes over Prow Jobs.

For the source of the state, I could handle that in the next PR

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 10, 2018

Member

well the inconsistent state that is more concerning than "it'll show up in a moment" is when we fail to post a status to github or a conflicting status is posted vs the prowjob (this can happen for various reasons, including network flakes, and that statuses are keyed by [repo, commit] which can be in more than one PR...)

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 10, 2018

Author Member

Got it. I can see it will be a very rare case that Prow Job would be used; how about displaying github contexts only?

This comment has been minimized.

Copy link
@stevekuznetsov

stevekuznetsov Jul 18, 2018

Contributor

That looks great!

This comment has been minimized.

Copy link
@qhuynh96

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 18, 2018

Member

bump, I think this makes sense with some hover text, can we get rid of the black behind the icon?

This comment has been minimized.

Copy link
@qhuynh96

qhuynh96 Jul 18, 2018

Author Member

Yeah, I can remove the black. Text hovering is already there but it wears off at the moment I take the screenshot

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 18, 2018

Member

thanks :-)
we had some problem with the 1.11.1 release images that i'm looking at now, but I'm leaving this open in a tab to review tomorrow. I want to get this in and deployed :-)

@k8s-ci-robot k8s-ci-robot requested a review from BenTheElder Jul 9, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Jul 9, 2018

Thanks for working on this!

@MHBauer

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

in example screenshots, is the CLA the github status that's new and added by this PR?

@qhuynh96

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2018

Yes. And not only CLA but all of the contexts above are Github statuses as ProwJobs were expired already.

func (da *DashboardAgent) HeadContexts(ghc githubClient, pr PullRequest) ([]Context, error) {
org := string(pr.Repository.Owner.Login)
repo := string(pr.Repository.Name)
combined, err := ghc.GetCombinedStatus(org, repo, string(pr.HeadRefOID))

This comment has been minimized.

Copy link
@BenTheElder

BenTheElder Jul 19, 2018

Member

I think this is fine for this PR, but cc @cjwagner IIRC we have to do some other tricks in tide to reliably get the statuses

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 19, 2018

@BenTheElder
Copy link
Member

left a comment

/lgtm
Thanks again @qhuynh96! 🎉

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, qhuynh96

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 merged commit 1c63658 into kubernetes:master Jul 19, 2018

11 checks passed

cla/linuxfoundation qhuynh96 authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-config Skipped
pull-test-infra-verify-deps Skipped
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.