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

Guide for monitoring tests run by CI - final push #5449

Merged
merged 26 commits into from
Aug 17, 2021

Conversation

RobertKielty
Copy link
Member

Re-submission of the Test Monitoring Guide written by @alejandrox1

We will apply feedback as requested by @justaugustus here on the original PR

Origin thread on Slack

@spiffxp @thejoycekung @hasheddan @jeremyrickard @justaugustus

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/developer-guide Issues or PRs related to the developer guide labels Jan 30, 2021
@k8s-ci-robot k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Jan 30, 2021
@RobertKielty RobertKielty changed the title Added guide for monitoring CI Guide for monitoring test run by CI - final push Jan 30, 2021
@RobertKielty RobertKielty changed the title Guide for monitoring test run by CI - final push Guide for monitoring tests run by CI - final push Jan 30, 2021
@RobertKielty
Copy link
Member Author

/test all

@RobertKielty RobertKielty marked this pull request as ready for review February 2, 2021 17:30
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 2, 2021
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

Agree with @thejoycekung's review, made a few suggestions

contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 9, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Feb 11, 2021
Co-authored-by: Joyce Kung <[email protected]>
Co-authored-by: Jorge Alarcon Ochoa <[email protected]>
Copy link
Contributor

@thejoycekung thejoycekung left a comment

Choose a reason for hiding this comment

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

Been meaning to look through the latest set of changes for a bit. Thanks for all your work so far @RobertKielty !

contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
Comment on lines +28 to +29
results in a grid. TestGrid's back end components are open sourced and can be
viewed in the [TestGrid repo] The front-end code
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: unsure which is the correct way, but back end vs front-end should be consistent (i.e. use a space for both or use a hyphen for both).

contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
**Note**: It is important that all SIGs periodically monitor their jobs and
tests. Furthermore, if jobs or tests are failing or flaking, then pull requests
will take a lot longer to be merged. For more information on how flaking tests
disrupt PR merging and how to eliminate them see [Flaky Tests]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
disrupt PR merging and how to eliminate them see [Flaky Tests]
disrupt PR merging and how to eliminate them see [Flaky Tests].


If your contributions involve code for past releases of kubernetes (e.g.
cherry-picks or backports), we recommend you periodically check on the
*blocking* and *informing* dashboards for [past releases]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
*blocking* and *informing* dashboards for [past releases]
*blocking* and *informing* dashboards for [past releases].

contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Outdated Show resolved Hide resolved
contributors/devel/sig-testing/monitoring.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 26, 2021
Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

/approve
/hold
I think we're about at merge and iterate so I'm going to approve.

But I defer to @thejoycekung for the final say, who continues to raise valid suggestions. Remove hold when you're ready to lgtm and thanks for the thorough reviews!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RobertKielty, spiffxp

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2021
@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 28, 2021
@ramrodo
Copy link
Member

ramrodo commented Jul 1, 2021

But I defer to @thejoycekung for the final say, who continues to raise valid suggestions. Remove hold when you're ready to lgtm and thanks for the thorough reviews!

Hi @thejoycekung, doing a follow-up of this PR, do you have more suggestions or this PR is ready to be merged?

@thejoycekung
Copy link
Contributor

hey @ramrodo thanks for following up! I remember chatting with @RobertKielty about this a few weeks ago -- from what I can tell there are still a few unaddressed comments, but once those are fixed I'd love to push this through :)

@justaugustus
Copy link
Member

Looks like this got stuck.
Let's please take any outstanding review comments as follow-ups.

/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 17, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2021
@justaugustus
Copy link
Member

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 17, 2021
@k8s-ci-robot k8s-ci-robot merged commit a50309d into kubernetes:master Aug 17, 2021
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. area/developer-guide Issues or PRs related to the developer guide cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants