Expose malformed status contexts on PR HEAD commit #7454

stevekuznetsov · 2018-03-28T16:51:09Z

When all labels are appropriately present or absent on a PR, tide will
still not merge the PR unless all status contexts are passing. We need
to expose non-passing status contexts on the HEAD commit of the PR so
this is clear to users.

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

Fixes #7449
/area prow
/kind bug
/cc @Kargakis @fejta
/assign @cjwagner @BenTheElder

stevekuznetsov · 2018-03-28T16:58:21Z

I didn't use headContexts() since this seems like a best-effort sort of thing and a waste of tokens if we end up with PRs that we need to call out for. But that can be refactored, I don't feel strongly.

0xmichalis · 2018-03-28T19:53:43Z

prow/tide/tide.go

+			sort.Strings(malformedContexts)
+			trunced := truncate(malformedContexts)
+			if len(trunced) == 1 {
+				desc = fmt.Sprintf(" Job %s has not succeeded.", trunced[0])


Hrm, what about the cla check context?

Don't we block on it today?

Nevermind, I guess it's a job.

0xmichalis · 2018-03-28T19:54:28Z

prow/tide/tide.go

+		var malformedContexts []string
+		for _, commit := range pr.Commits.Nodes {
+			if commit.Commit.OID == pr.HeadRefOID {
+				for _, status := range commit.Commit.Status.Contexts {


Do you also pick up the tide context?

Good catch, yes

add this case to the unit tests?

fejta · 2018-03-28T22:51:03Z

prow/tide/tide.go

+			if commit.Commit.OID == pr.HeadRefOID {
+				for _, status := range commit.Commit.Status.Contexts {
+					if status.State != githubql.StatusStateSuccess {
+						malformedContexts = append(malformedContexts, string(status.Context))


Why are unsuccessful contexts malformed? This makes me think they are corrupted

fejta · 2018-03-28T22:52:50Z

prow/tide/tide.go

+		var malformedContexts []string
+		for _, commit := range pr.Commits.Nodes {
+			if commit.Commit.OID == pr.HeadRefOID {
+				for _, status := range commit.Commit.Status.Contexts {


add this case to the unit tests?

stevekuznetsov · 2018-03-29T16:32:33Z

@fejta @Kargakis updated

@cjwagner how do you feel about this being "best-effort" or would you rather see me use headContexts() here?

cjwagner · 2018-03-29T16:52:23Z

@stevekuznetsov The current usage of headContexts could already be prohibitively expensive for k/k...

I'd prefer to find a way to make headContexts less wasteful:

We could do this trivially with a proxy cache layer.
We could cache the statuses for a commit, but only if we make tide receive status webhooks and invalidate the cache entry for a commit when a status is updated. This isn't a good option because if we miss a status webhook, tide could operate on stale data.

stevekuznetsov · 2018-03-29T21:12:46Z

@cjwagner OK, that is understood. I think those approaches are probably useful, but orthogonal to the changes here. Given your response I continue to be confident that not using the headContexts() call is appropriate in the status controller.

The test flaked on:

W0329 16:33:27.573] ERROR: (gcloud.auth.activate-service-account) There was a problem refreshing your current auth tokens: Unable to find the server at accounts.google.com

/retest

@cjwagner @fejta any other comments?

cjwagner · 2018-03-29T21:24:00Z

prow/tide/tide.go

@@ -246,11 +247,34 @@ func requirementDiff(pr *PullRequest, q *config.TideQuery) (string, int) {
 			desc = fmt.Sprintf(" Should not have %s labels.", strings.Join(trunced, ", "))
 		}
 	}
+	diff = len(missingLabels) + len(presentLabels)
+
+	if desc == "" {


We still need to check for unsuccessful contexts in order to return a correct diff even if we already have a message to use for the status description.

cjwagner · 2018-03-29T21:26:28Z

prow/tide/tide.go

+				}
+			}
+		}
+		if len(contexts) > 0 {


if desc == "" && len(contexts) > 0 {

cjwagner · 2018-03-29T21:26:57Z

prow/tide/tide.go

+			} else {
+				desc = fmt.Sprintf(" Jobs %s have not succeeded.", strings.Join(trunced, ", "))
+			}
+			diff = len(contexts)


When all labels are appropriately present or absent on a PR, `tide` will still not merge the PR unless all status contexts are passing. We need to expose non-passing status contexts on the `HEAD` commit of the PR so this is clear to users. Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>

stevekuznetsov · 2018-03-29T22:21:56Z

@cjwagner your approach to the diff count makes a lot of sense. Updated.

cjwagner

/lgtm

k8s-ci-robot · 2018-03-29T23:15:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, stevekuznetsov

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:

~~prow/OWNERS~~ [cjwagner,stevekuznetsov]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

stevekuznetsov requested a review from cjwagner as a code owner March 28, 2018 16:51

k8s-ci-robot assigned BenTheElder and cjwagner Mar 28, 2018

k8s-ci-robot added area/prow Issues or PRs related to prow kind/bug Categorizes issue or PR as related to a bug. labels Mar 28, 2018

k8s-ci-robot requested review from fejta and 0xmichalis March 28, 2018 16:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. 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. labels Mar 28, 2018

0xmichalis reviewed Mar 28, 2018

View reviewed changes

fejta reviewed Mar 28, 2018

View reviewed changes

stevekuznetsov force-pushed the skuznets/report-contexts branch from fce8950 to 33e7dc4 Compare March 29, 2018 16:32

cjwagner reviewed Mar 29, 2018

View reviewed changes

cjwagner mentioned this pull request Mar 29, 2018

Allow branches to be excluded in tide queries #7456

Merged

stevekuznetsov force-pushed the skuznets/report-contexts branch from 33e7dc4 to 1ac67f2 Compare March 29, 2018 22:21

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 29, 2018

cjwagner approved these changes Mar 29, 2018

View reviewed changes

k8s-ci-robot merged commit 91135c5 into kubernetes:master Mar 29, 2018

sebastienvas added a commit to sebastienvas/k8s-test-infra that referenced this pull request Mar 30, 2018

Fix merge confict with kubernetes#7454

43f8769

sebastienvas mentioned this pull request Mar 30, 2018

Adding a Tide Configuration for checking only github required checks. #7457

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose malformed status contexts on PR HEAD commit #7454

Expose malformed status contexts on PR HEAD commit #7454

stevekuznetsov commented Mar 28, 2018

stevekuznetsov commented Mar 28, 2018

0xmichalis Mar 28, 2018

stevekuznetsov Mar 28, 2018

0xmichalis Mar 29, 2018

0xmichalis Mar 28, 2018

stevekuznetsov Mar 28, 2018

fejta Mar 28, 2018

fejta Mar 28, 2018

fejta Mar 28, 2018

stevekuznetsov commented Mar 29, 2018

cjwagner commented Mar 29, 2018

stevekuznetsov commented Mar 29, 2018

cjwagner Mar 29, 2018

cjwagner Mar 29, 2018

cjwagner Mar 29, 2018

stevekuznetsov commented Mar 29, 2018

cjwagner left a comment

k8s-ci-robot commented Mar 29, 2018

Expose malformed status contexts on PR HEAD commit #7454

Expose malformed status contexts on PR HEAD commit #7454

Conversation

stevekuznetsov commented Mar 28, 2018

stevekuznetsov commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Mar 29, 2018

cjwagner commented Mar 29, 2018

stevekuznetsov commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov commented Mar 29, 2018

cjwagner left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 29, 2018