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

Nitpicking the 8.0 Breaking Change issue template #81678

Merged
merged 2 commits into from
Oct 26, 2020
Merged

Nitpicking the 8.0 Breaking Change issue template #81678

merged 2 commits into from
Oct 26, 2020

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Oct 26, 2020

PR author's comments are inline.

@kobelb kobelb requested a review from cjcenizal October 26, 2020 20:49
@kobelb kobelb added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 26, 2020
kobelb
kobelb commented Oct 26, 2020
View reviewed changes
.github/ISSUE_TEMPLATE/v8_breaking_change.md
@@ -2,7 +2,7 @@
name: 8.0 Breaking change
about: Breaking changes from 7.x -> 8.0
title: "[Breaking change]"
labels: Team:Elasticsearch UI, Feature:Upgrade Assistant
labels: Team:Elasticsearch UI, Feature:Upgrade Assistant, Breaking Change
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the "Breaking Change" label to differentiate these from the other issues associated with the Upgrade Assistant. This would be a new label, I'm not tied to the name. I also considered making it Feature:Upgrade Assistant - Breaking Change. Any preference or thoughts on a different way to classify the two different types of issues?

Copy link
Contributor

Choose a reason for hiding this comment

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

Love it! What do you think of using the "Breaking change" label as a dimension for categorizing it by how it will be surfaced? We can create three labels:

Breaking Change
Breaking Change:Deprecation Log
Breaking Change:Deprecation API

By default, we can apply Breaking Change via this template. As these issues are created we'll triage them by swapping this label out for one of the others. Or do you think this overloads the label too much / adds too much complexity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me!

.github/ISSUE_TEMPLATE/v8_breaking_change.md
@@ -11,15 +11,15 @@ assignees: ''

**Which release will ship the breaking change?**

<!-- e.g., v7.6.2 -->
8.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue template states that it's an 8.0 breaking change, so it feels like this will be 8.0 for all of these? Or perhaps we should change Which release will ship the breaking change? to Which release first included the deprecation?, but I'm not sure how the answer to this new question impacts anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call hardcoding this in here. I don't think we'll ever encounter a scenario where we'll need to change this between now and the time 8.0 is GA.

.github/ISSUE_TEMPLATE/v8_breaking_change.md Outdated
@@ -11,15 +11,15 @@ assignees: ''

**Which release will ship the breaking change?**

<!-- e.g., v7.6.2 -->
8.0

**Describe the change. How will it manifest to users?**

**What percentage of users will be affected?**

<!-- e.g., Roughly 75% will need to make changes to x. -->
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is hard to answer if we don't have telemetry data, which is unfortunately rather common. I feel like we should be differentiating "guesses" from "data-driven" percentages... Thoughts? I answered the following when creating #81674

Based on data: ??
Based on hunches: a lot

Copy link
Contributor

@cjcenizal cjcenizal Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Good point. I think we should change this question to be "What's the impact of this breaking change?" or "How many users will be impacted by this breaking change?" and then use the comment to prompt the author to explain their reasoning and whether they have data to measure the impact or some other heuristic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, sounds good. Lemme give it a shot real quick

cjcenizal
cjcenizal approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

LGTM!

@kobelb kobelb requested a review from cjcenizal October 26, 2020 21:47
cjcenizal
cjcenizal approved these changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Looks Even Gooder To Me

@kobelb kobelb merged commit 34af716 into elastic:master Oct 26, 2020
@kobelb kobelb deleted the 8-0-breaking-change-template branch October 26, 2020 21:59
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 27, 2020
@gmmorris
Merge branch 'master' into task-manager/health
314f3bc 
* master: (37 commits)
  [ILM] Migrate Warm phase to Form Lib (elastic#81323)
  [Security Solutions][Detection Engine] Fixes critical bug with error reporting that was doing a throw (elastic#81549)
  [Detection Rules] Add 7.10 rules (elastic#81676)
  [kbn/optimizer] ignore missing metrics when updating limits with --focus (elastic#81696)
  [SECURITY SOLUTIONS] Bugs overview page + investigate eql in timeline (elastic#81550)
  [Maps] fix unable to edit cluster vector styles styled by count when switching to super fine grid resolution (elastic#81525)
  Fixed migration issue for case specific actions, by extending email action migrator checks (elastic#81673)
  [CI] Preparation for APM tracking on CI (elastic#80399)
  [Home] Fixes Kibana app description order on home page and updates Canvas copy (elastic#80057)
  Make sure `to` is 'now' and not the same as `from` (elastic#81524)
  Nitpicking the 8.0 Breaking Change issue template (elastic#81678)
  [SECURITY_SOLUTION] Fix text on onboarding screen (elastic#81672)
  [data.search] Skip async search tests in build candidates and production builds (elastic#81547)
  Fix previousStartedAt by not changing when execution fails (elastic#81388)
  [Monitoring] Fix a couple of issues with the cpu usage alert (elastic#80737)
  Telemetry collection xpack to ts project references (elastic#81269)
  Elasticsearch: don't use url authentication for new client (elastic#81564)
  [App Search] Credentials: implement working flyout form (elastic#81541)
  Properly encode links to edit user page (elastic#81562)
  [Alerting UI] Don't wait for health check before showing Create Alert flyout (elastic#80996)
  ...
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2020
@kibanamachine
Copy link
Contributor

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

@cjcenizal cjcenizal added the backport:skip This commit does not require backporting label Oct 28, 2020
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjcenizal cjcenizal cjcenizal approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@kobelb @kibanamachine @cjcenizal