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

Flakefinder v2 - implement several reports #168

Merged
merged 37 commits into from
Aug 29, 2019

Conversation

dhiller
Copy link
Contributor

@dhiller dhiller commented Aug 22, 2019

As said here flakefinder needs some more work, so this PR (based on #146) has been created to work on it.

Note: after #146 has been merged, this PR needs a rebase

Goal: extend the flakefinder so that it produces reports for:

  • sum of 4 weeks
  • sum of last week and
  • in addition e.g. the last 3 days individually

rmohr and others added 17 commits August 22, 2019 12:34
bazel go module support is not yet that great
Write report to kubevirt bucket under
/reports/flakefinder/flakefinder-$isodate.html, then create index.html
as wrapping page. This page contains the links to the last 50 reports.
NOTE: this is not yet working, `bazel build //robots/flakefinder`
yields a compile error that I need support with to understand and
fix.
As the job is analyzing kubevirt repo, it belongs to this group,
therefore it is moved there. Remove other definitions for cleanup.
- Pin image id
- Remove unnecessary build steps
- Add reasonable title attribute to html report files
- Add details to README
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2019
- 24h -> daily / 168h -> weekly / 672h -> four-weekly
- Convert report index colspan into header to avoid further problems
@dhiller
Copy link
Contributor Author

dhiller commented Aug 26, 2019

ATM the report index page looks like this:
image

So, as you might guess there are three report spans here (24h, 1 week, and 4 weeks). Of course not really well designed, but should still be useful.

Fix test target, add injection of latest image sha id into job yaml
- Export type for experted method
- Remove unused type
- export MkaeQuery, add test
Copy link
Contributor

@slintes slintes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great already, some small comments inline.

Not sure if it's worth for such a small application, I won't block on this, but I think from a structural POV this can be improved a bit:

  • create cmd and pkg packages and move files there
  • for e.g. downloader.go: create a Downloader struct, which can be initialized with NewDownloader(...) and takes the context, client etc., and make the funcs methods of that struct

But it's fine for me as it is. This can be done in a follow up, or in case the application grows.

@@ -0,0 +1,74 @@
flakefinder
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice readme 👍


### Build

bazel run //robots/flakefinder:app
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe worth making build and push make targets?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will come in the next commits. :)

return res
}

type finishedStatus struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems to be unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good spot, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Goland spotted it ;)

ReportFiles map[ReportFileMergedDuration]string
}

type reportFile struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused?

return nil, fmt.Errorf("Cannot read started.json (%s) in bucket '%s'", dirOfStartedJSON, bucket)
}
if !IsLatestCommit(startedJSON, pr) {
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as I saw this is correct as is. I'm currently debugging some kind of mapping problem there.

Added log message if it can't find junit xml, cleaned formatting,
added tests for report output.
@dhiller
Copy link
Contributor Author

dhiller commented Aug 27, 2019

So, the reason for the empty daily report is that the junit xml file can't be found. Interestingly this is the case for all the merged PRs since yesterday.

@dhiller
Copy link
Contributor Author

dhiller commented Aug 27, 2019

I think I found it. @slintes you were partially right.

@dhiller dhiller changed the title [WIP] Flakefinder v2 - implement several reports Flakefinder v2 - implement several reports Aug 29, 2019
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2019
@@ -23,6 +23,9 @@ import (
"context"
"flag"
"fmt"
"github.com/google/go-github/v28/github"
"github.com/sirupsen/logrus"
"golang.org/x/oauth2"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: move these new imports down

@@ -42,7 +44,7 @@ func flagOptions() options {
flag.DurationVar(&o.merged, "merged", 24*7*time.Hour, "Filter to issues merged in the time window")
flag.Var(&o.endpoint, "endpoint", "GitHub's API endpoint")
flag.StringVar(&o.token, "token", "", "Path to github token")
flag.StringVar(&o.graphqlEndpoint, "graphql-endpoint", github.DefaultGraphQLEndpoint, "GitHub's GraphQL API Endpoint")
//flag.StringVar(&o.graphqlEndpoint, "graphql-endpoint", github.DefaultGraphQLEndpoint, "GitHub's GraphQL API Endpoint")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if not needed, please remove

for _, issue := range issues {
pr, err := c.GetPullRequest("kubevirt", "kubevirt", issue.Number)
for nextPage := 1; nextPage > 0; {
issues, response, err := c.Search.Issues(ctx, query, &github.SearchOptions{ListOptions: github.ListOptions{Page: nextPage}})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new client can do c.PullRequests.List(...), any reason to stay with c.Search.Issues(....) and then get every single PR?

Copy link
Contributor Author

@dhiller dhiller Aug 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to search for merged PRs. I did not find out how to do this with PullRequest.List, or did I overlook something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed to use PullRequests.List.

Remove unused method, add comment on why starting from midnight
@slintes
Copy link
Contributor

slintes commented Aug 29, 2019

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 29, 2019
@slintes
Copy link
Contributor

slintes commented Aug 29, 2019

great work 👍

@dhiller
Copy link
Contributor Author

dhiller commented Aug 29, 2019

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhiller

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 29, 2019
@kubevirt-bot kubevirt-bot merged commit db3ccd8 into kubevirt:master Aug 29, 2019
@kubevirt-bot
Copy link
Contributor

@dhiller: Updated the job-config configmap in namespace kubevirt-prow using the following files:

  • key kubevirt-periodics.yaml using file github/ci/prow/files/jobs/kubevirt/kubevirt-periodics.yaml
  • key project-infra-presubmits.yaml using file github/ci/prow/files/jobs/project-infra/project-infra-presubmits.yaml

In response to this:

As said here flakefinder needs some more work, so this PR (based on #146) has been created to work on it.

Note: after #146 has been merged, this PR needs a rebase

Goal: extend the flakefinder so that it produces reports for:

  • sum of 4 weeks
  • sum of last week and
  • in addition e.g. the last 3 days individually

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.

gabrielecerami pushed a commit to gabrielecerami/project-infra that referenced this pull request Oct 7, 2020
* cluster: add support for different runtimes

with this patch, we open the road for different helper tools in addition
to the standard `gocli`, which is still the default choice.
We want to make room for the future podman support, which is currently
experimented with in `github.com/fromanirh/pack8s`, and this seems
the safest and less invasive way.

Signed-off-by: Francesco Romani <fromani@redhat.com>

* cluster: move container var under conditional

We use the _cli_container var iff we want to use
the stable `gocli` tool, hence move the variable declaration
under the if branch.

Signed-off-by: Francesco Romani <fromani@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants