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

add pr history dashboard to Deck #10169

Merged
merged 1 commit into from Dec 18, 2018

Conversation

@ibzib
Copy link
Collaborator

ibzib commented Nov 16, 2018

Doc (shared with sig-testing)

Implements a PR history dashboard (a la Gubernator) in Deck to show all Prow jobs triggered by a certain PR.

/assign @cjwagner @Katharine
/cc @krzyzacy @BenTheElder @AishSundar

screen shot 2018-11-16 at 3 51 27 pm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 16, 2018

Hi @ibzib. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@cjwagner

This comment has been minimized.

Copy link
Member

cjwagner commented Nov 17, 2018

/ok-to-test

@ibzib ibzib force-pushed the ibzib:job-history branch 2 times, most recently from ea4110d to 1c6fafa Nov 17, 2018

@ibzib ibzib force-pushed the ibzib:job-history branch from 1c6fafa to 2f41ff6 Nov 17, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Nov 17, 2018

/lint

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot left a comment

@BenTheElder: 0 warnings.

In response to this:

/lint

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.

Show resolved Hide resolved prow/config/config.go Outdated
@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Nov 17, 2018

First pass:

  • functionality looks great
  • UI looks good 👍
  • could do with some more code comments

@ibzib ibzib force-pushed the ibzib:job-history branch from 02f7002 to 8bf46e0 Nov 17, 2018

Show resolved Hide resolved prow/cmd/deck/job_history.go Outdated
Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
bd := make([]map[string][]buildData, len(jobNames))
width := make([]map[string]int, len(jobNames))

// concurrently find which jobs have been run on this PR and their build numbers

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Generally we bound the number of workers on things like this to keep the resource use per request bounded

This comment has been minimized.

@ibzib

ibzib Nov 29, 2018

Collaborator

Resource usage here is directly proportional to the number of prow jobs a single PR has triggered. It seems unlikely that number would be more than a few dozen. In the event that it does actually present a problem, we can limit it, but right now it doesn't seem necessary

Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
return
}

func getPRHistory(url *url.URL, config *config.Config, gcsClient *storage.Client) (prHistoryTemplate, error) {

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 27, 2018

Contributor

Can we break this function up into components and unit test each one?

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 17, 2018

Contributor

This comment seems outstanding.

Show resolved Hide resolved prow/config.yaml Outdated

@ibzib ibzib force-pushed the ibzib:job-history branch from 1f75c50 to f5961c0 Nov 30, 2018

func parsePRHistKey(key, defaultOrg, defaultRepo string) (bucketName, org, repo, prNumber string, err error) {
match := prRe.FindStringSubmatch(key)
if len(match) != 5 {
link := "https://github.com/kubernetes/test-infra/tree/master/gubernator#gcs-bucket-layout"

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 30, 2018

Contributor

We should migrate that out of Gubernator at some point, too

Show resolved Hide resolved prow/cmd/deck/pr_history.go Outdated
start := time.Now()
template := prHistoryTemplate{}

key := strings.TrimPrefix(url.Path, "/pr-history/")

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Nov 30, 2018

Contributor

How did we choose this to be the input a user sends to the endpoint?

This comment has been minimized.

@ibzib

ibzib Nov 30, 2018

Collaborator

It's kind of arbitrary, do you have something better in mind?

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 4, 2018

Contributor

Do we know the PJ name or can we use query parameters? I really am not a fan of the (convoluted) GCS layout string and in general not a fan of taking structured data, flattening it to a string, then parsing it back out.

This comment has been minimized.

@Katharine

Katharine Dec 4, 2018

Member

In this context there isn't a PJ name to know - we're going to fetch all the jobs that executed for a given PR from GCS.

Given that these URLs are public-facing, I would prefer to avoid a series of query parameters.

That said, I wonder if it's viable to go the other way: given a path like /pr-history/[org]/[repo]/[pr] or /pr-history/[repo]/[pr], figure out where the bucket should be. This would probably make it significantly easier to generate a URL to this page, match gubernator's approach, and be cleaner for users.

The key difficulty with that approach is that it requires you to know the correct bucket for an org/repo, which may not necessarily be the default. This isn't a problem kubernetes has today (since kubernetes-security is not publicly visible and AFAIK everything else this prow serves is in the default bucket), but it is a possible configuration and I don't think we have a good way to do a lookup from orgs/repos to buckets.

I personally think we should either punt making the URLs better or punt supporting non-default buckets to some future time, depending on whether we think anyone actually uses non-default buckets in a single prow and expects them to be publicly viewable.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 4, 2018

Contributor

Given that these URLs are public-facing, I would prefer to avoid a series of query parameters.

What? I seriously doubt anyone is going to be entering these by hand. They're clicking links in a GUI. I don't think we should care about this.

This would probably make it significantly easier to generate a URL to this page, match gubernator's approach, and be cleaner for users.

The most technically correct approach here is to ingest the org, repo, and PR. Use the PR to determine the target branch for it, then use the loaded job configuration to determine which jobs would have ran. Use the Plank configuration and the job configuration to determine which GCS buckets those jobs would have pushed to.

depending on whether we think anyone actually uses non-default buckets in a single prow and expects them to be publicly viewable.

We do, Gubernator supports it and we should expect this to work.

This comment has been minimized.

@Katharine

Katharine Dec 4, 2018

Member

What? I seriously doubt anyone is going to be entering these by hand. They're clicking links in a GUI. I don't think we should care about this.

I think clean URLs have multiple upsides: they're easier to manipulate (users do do this! sometimes it's easier than clicking through the right maze of links), they're easier to generate, and they're harder to screw up.

That said...

The most technically correct approach here is to ingest the org, repo, and PR. Use the PR to determine the target branch for it, then use the loaded job configuration to determine which jobs would have ran. Use the Plank configuration and the job configuration to determine which GCS buckets those jobs would have pushed to.

I looked into this, drafted an implementation, and it seems reasonable enough to do. I believe @ibzib is working on taking this approach now. If we do this, we'll instead have URLs like /pr-history/kubernetes/test-infra/12345. Is that acceptable?

We do, Gubernator supports it and we should expect this to work.

That's entirely fair.

This comment has been minimized.

@ibzib

ibzib Dec 4, 2018

Collaborator

+1 Parsing long strings using regular expressions is admittedly gross and probably error-prone. We should probably change the job history page to also use this strategy.

This comment has been minimized.

@stevekuznetsov

stevekuznetsov Dec 4, 2018

Contributor

Breaking out org and repo and explicitly placing both in the URL is totally reasonable. I just don't want us to perpetuate the optional org/optional repo and underscore nonsense unless we need to.

@stevekuznetsov

This comment has been minimized.

Copy link
Contributor

stevekuznetsov commented Dec 7, 2018

Let's remember to squash this before merge
/hold
:)

@ibzib

This comment has been minimized.

Copy link
Collaborator

ibzib commented Dec 7, 2018

Updates:

  • simplified paths (/pr-history/<org>/<repo>/<pr>)
  • logic broken up into helper functions wherever possible
  • lots of unit tests added
  • rebased
    @stevekuznetsov @Katharine give it a final pass 🙏

@ibzib ibzib force-pushed the ibzib:job-history branch from d3d55ef to 9f76e0c Dec 10, 2018

@BenTheElder
Copy link
Member

BenTheElder left a comment

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 15, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 15, 2018

LGTM label has been added.

Git tree hash: 85de6ed053a8afe1071428458a59e59a7f3c2d5f

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 15, 2018

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Dec 15, 2018

oops! forgot to trigger the lint bot first 😈
/lint

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot left a comment

@BenTheElder: 0 warnings.

In response to this:

oops! forgot to trigger the lint bot first 😈
/lint

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.

@ibzib

This comment has been minimized.

Copy link
Collaborator

ibzib commented Dec 18, 2018

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 79a51c3 into kubernetes:master Dec 18, 2018

12 checks passed

cla/linuxfoundation ibzib 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-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
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

@ibzib ibzib referenced this pull request Dec 19, 2018

Closed

REQUEST: New membership for @ibzib #316

6 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment