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.

Sign up for GitHub

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

Jump to bottom

Add "Risk Matrix" section to the PR template #100649

Merged
merged 15 commits into from
Jun 2, 2021
Merged

Add "Risk Matrix" section to the PR template #100649

merged 15 commits into from
Jun 2, 2021

Conversation

streamich
Copy link
Contributor

@streamich streamich commented May 26, 2021 •
edited
Loading

This adds a "Risk Matrix" section to the PR template proposed in e-mail thread by Petr.

👉 Rendered text

@streamich streamich changed the title docs: ✏️ add "Risk Matrix" section to PR template Add "Risk Matrix" section to the PR template May 26, 2021
@streamich streamich marked this pull request as ready for review May 26, 2021 12:00
@streamich streamich added release_note:skip Skip the PR/issue when compiling release notes review Team:AppServices v8.0.0 labels May 26, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServices)

stacey-gammon
stacey-gammon approved these changes May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
tsullivan
tsullivan reviewed May 26, 2021
View reviewed changes
Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

Just a few comments and a question. I like this!

kobelb
kobelb reviewed May 26, 2021
View reviewed changes
RISK_MATRIX.md Outdated Show resolved Hide resolved
RISK_MATRIX.md Outdated Show resolved Hide resolved
RISK_MATRIX.md Outdated Show resolved Hide resolved
RISK_MATRIX.md Outdated Show resolved Hide resolved
RISK_MATRIX.md Outdated Show resolved Hide resolved
jloleysens
jloleysens approved these changes May 27, 2021
View reviewed changes
Copy link
Contributor

@jloleysens jloleysens left a comment

Choose a reason for hiding this comment

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

Left one suggestion for a change to the RISK_MATRIX.md and a question about the PR template generally.

Not blocking on either but I think other feedback should be addressed before merging 👍

RISK_MATRIX.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
streamich and others added 6 commits May 27, 2021 14:54
@streamich @tsullivan
Update RISK_MATRIX.md
d8adb92 
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
@streamich @tsullivan
Update RISK_MATRIX.md
6ad0bc0 
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
@streamich @tsullivan
Update RISK_MATRIX.md
94eaa9f 
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
@streamich @tsullivan
Update RISK_MATRIX.md
c08ac21 
Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com>
@streamich @kobelb
Update RISK_MATRIX.md
0ff8ca4 
Co-authored-by: Brandon Kobel <brandon.kobel@gmail.com>
@streamich @kobelb
Update RISK_MATRIX.md
f70d1a1 
Co-authored-by: Brandon Kobel <brandon.kobel@gmail.com>
lukeelmers
lukeelmers reviewed Jun 1, 2021
View reviewed changes
Copy link
Member

@lukeelmers lukeelmers left a comment

Choose a reason for hiding this comment

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

Stumbled upon this PR from Stacey's email - It isn't immediately clear to me who this matrix is for. Is it a reminder for developers to test edge cases? A set of notes for reviewers? A decision-making tool for when a PR is ready to merge? All three? Who is responsible for updating it?

I ask because this feels like it belongs with our existing best practices documentation, and I wonder if there's a way to consolidate everything in one place so that we aren't updating 2 different locations with the same information.

A simpler alternative with a clear call to action could be a checkbox that says "This PR has been tested for the risks outlined in our best practices documentation"

RISK_MATRIX.mdx Show resolved Hide resolved
@streamich
Copy link
Contributor Author

streamich commented Jun 2, 2021 •
edited
Loading

Stumbled upon this PR from Stacey's email - It isn't immediately clear to me who this matrix is for. Is it a reminder for developers to test edge cases? A set of notes for reviewers? A decision-making tool for when a PR is ready to merge? All three? Who is responsible for updating it?

@lukeelmers Yes, it can be used for all three. The idea of this section comes from the idea that projects or big features can have a "Test Plan" document attached to them, but creating test plans is time-consuming and really the main thing in Kibana that we are missing from test plans is the "Risk Matrix". So, instead of requiring big features to create a "Test Plan" document, each sizeable enough PR should add the "Risk Matrix" section and follow our regular testing guidelines.

The "Risk Matrix" table is supposed to be updated by the developer, their team, other stakeholders, and the QA team. The idea is that for a sizeable feature more people (than just the developer who wrote it) think about potential risks. And then when writing tests or manually testing the "Risk Matrix" can be used as guidance. Finally, the "Risk Matrix" can be used for retrospection, when something goes wrong in production, we can look up the risks we considered or missed.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 256 146 -110
lens 67 45 -22
licensing 18 15 -3
lists 239 236 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
securitySolution 390 346 -44
stackAlerts 101 95 -6
total -340

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@stacey-gammon
Copy link
Contributor

@lukeelmers - I consider the asciidoc developer guide to be legacy and no longer maintained, although I don't think we should delete it until the new docs system is public. I'm checking with leads to see if we can make a more official announcement about that.

The file is mdx so we can link to it or embed it in Best Practices in the new developer guide.

@streamich streamich merged commit 1caaf47 into elastic:master Jun 2, 2021
@lukeelmers
Copy link
Member

instead of requiring big features to create a "Test Plan" document, each sizeable enough PR should add the "Risk Matrix" section and follow our regular testing guidelines.

I consider the asciidoc developer guide to be legacy and no longer maintained

@stacey-gammon @streamich Thanks for clarifying! I think these two bits are the context I was missing, makes more sense now.

The file is mdx so we can link to it or embed it in Best Practices in the new developer guide

+1 to linking to the new Best Practices (had no idea we had rewritten this!), and consolidating as much info as possible there. It will help reduce friction in onboarding new devs if they can find all of the best practices in one place.

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jun 2, 2021
@gmmorris
Merge branch 'master' into task-manager/capacity-estimation
2b9bd67 
* master: (68 commits)
  Unskip advanced settings a11y test (elastic#100558)
  [App Search] Crawler Landing Page (elastic#100822)
  [DOCS] Clarify when to use kbn clean (elastic#101155)
  change label behavior (elastic#100991)
  skip flaky suite (elastic#101126)
  Fix cases plugin ownership (elastic#101073)
  [Home] Adding file upload to add data page (elastic#100863)
  [ML] Functional tests - reenable categorization tests (elastic#101137)
  [DOCS] Adds server.uuid to settings docs (elastic#101121)
  Fix newsfeed unread notifications always on when reloading Kibana (elastic#100357)
  [Lens] Time shift metrics (elastic#98781)
  [Deprecations service] make `correctiveActions.manualSteps` required (elastic#100997)
  Add "Risk Matrix" section to the PR template (elastic#100649)
  [Maps] spatially filter by all geo fields (elastic#100735)
  [Security Solution] Add Ransomware canary advanced policy option (elastic#101068)
  [Exploratory view] Core web vitals (elastic#100320)
  [Security solution][Endpoint] Add unit tests for fleet event filters/trusted apps cards (elastic#101034)
  [Lens] Use a setter function for the dimension panel (elastic#101123)
  [Index Patterns] Fix return saved index pattern object (elastic#101051)
  [CI] For PRs, build TS refs before public api docs check (elastic#100791)
  ...
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jun 4, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@timroes
Copy link
Contributor

timroes commented Jun 8, 2021 •
edited
Loading

I want to second what look said above:

A simpler alternative with a clear call to action could be a checkbox that says "This PR has been tested for the risks outlined in our best practices documentation"

I stumbled across the new template the first time today, and I think our PR template is fulfilling less and les spurpose over time. We were in the past incredible careful about what we add there, because the more we add, the more likely people will simply ignore it or delete it without reading altogether.

This change adds a huge block of text now in there, that increases this risk even more, while everything the text actually refers to is: Make sure you've tested all edge-cases we know about. I think we should try to shorten that as luke suggested into a single check list item on the list and refer to another place to read more. I don't think anyone will suddenly be more aware of the risks, because we put the lengthy description inside the PR template instaed of keeping it in it's separate page.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

7 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 100649 or prevent reminders by adding the backport:skip label.

@lukeelmers lukeelmers added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kobelb kobelb kobelb left review comments

@tsullivan tsullivan tsullivan left review comments

@lukeelmers lukeelmers lukeelmers left review comments

@jportner jportner jportner left review comments

@jloleysens jloleysens jloleysens approved these changes

@stacey-gammon stacey-gammon stacey-gammon approved these changes

@ppisljar ppisljar Awaiting requested review from ppisljar

@petrklapka petrklapka Awaiting requested review from petrklapka

@LeeDr LeeDr Awaiting requested review from LeeDr

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes review v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@streamich @elasticmachine @kibanamachine @stacey-gammon @lukeelmers @timroes @kobelb @tsullivan @jportner @jloleysens