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

Recording breaking changes #37689

Closed
richlander opened this issue Jun 10, 2020 · 10 comments
Closed

Recording breaking changes #37689

richlander opened this issue Jun 10, 2020 · 10 comments

Comments

@richlander
Copy link
Member

richlander commented Jun 10, 2020

Recording breaking changes

Please follow the following guidance when making breaking changes. This guidance doesn't include validating that it is OK to make a breaking change. That's what the PR process is for.

  1. Create an issue that describes the breaking change, in detail. Things to consider including
    • [Required] Mark the issue with the breaking-change label.
    • Goals and motivation for the change
    • Pre-change behavior
    • Post-change behavior
    • Errors or other behavior you can expect when running old code that breaks
    • Workarounds and mitigations
  2. Create a docs issue, according to the docs template, and link to the issue created in step 1.
  3. [Optional] Mark associated PRs with the breaking-change label.

Examples

The following are good examples of breaking change issues

Other changes to consider

The repo README does not include any information of discovery high-value issues. We should add a query for current release breaking changes, among others.

RFC

This issue is currently an RFC. I wanted to get feedback on this process.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Meta untriaged New issue has not been triaged by the area owner labels Jun 10, 2020
@AaronRobinsonMSFT
Copy link
Member

It would also be good to include a "workaround" or "mitigations". The idea here is provide "next steps" for people hitting the breaking change. I would imagine that "There are no workarounds" is possible, but I would expect we can do better than that most of the time.

@AaronRobinsonMSFT
Copy link
Member

/cc @PriyaPurkayastha

@AaronRobinsonMSFT AaronRobinsonMSFT removed the untriaged New issue has not been triaged by the area owner label Jun 10, 2020
@richlander
Copy link
Member Author

Added "Workarounds and mitigations". Good?

@stephentoub
Copy link
Member

stephentoub commented Jun 10, 2020

There's already a breaking-change label that's been used on PRs:
https://github.com/dotnet/runtime/pulls?q=is%3Apr+label%3Abreaking-change+
in addition to being used on issues:
https://github.com/dotnet/runtime/issues?q=label%3Abreaking-change+
Are you proposing this as a replacement?
cc: @danmosemsft, @jeffschwMSFT

@stephentoub
Copy link
Member

stephentoub commented Jun 10, 2020

In your list of work, one thing is marked as required and one is marked as optional, but there are a bunch of other items in the checklist that aren't marked as required nor optional. How should I interpret that?
image

@stephentoub
Copy link
Member

Create an issue

What is the purpose of the issue? You write "This guidance doesn't include validating that it is OK to make a breaking change. That's what the PR process is for.", so presumably your intent is the issue is opened after the breaking change has already been PR'd, in which case the issue is just for documentation purposes? When is the issue closed? Is it just opened and immediately closed, and just exists to have something to link to?

@stephentoub
Copy link
Member

stephentoub commented Jun 10, 2020

More generally, this seems like a lot of ceremony, with a single change incurring a PR with a breaking change label, an issue in the runtime repo, and an issue in the docs repo. Obviously the PR is needed, and it should be in the docs in some way. Why is a runtime issue also needed? Seems very duplicative.

@danmoseley
Copy link
Member

danmoseley commented Jun 10, 2020

I have the same feedback as @stephentoub. Don't we have sufficient process? Adding more likely means it won't be consistently followed.

Also, would it be better to avoid starting with "Please follow the following guidance..." before achieving consensus? @PriyaPurkayastha you're the.NET compat champ, is this an ask from you to change your process?

@PriyaPurkayastha
Copy link

@danmosemsft No, this is not an ask from me.

Probably Rich is not aware of the breaking change process that is in place since .NET Core 3.0 timeframe. I have seen some team members following the process and others do not seem to know about it, so maybe it would be helpful if I copy/paste relevant blurb from the .NET 5 Compat principles and guidance document below:

Proactive approach for compat breaking changes:
Teams can proactively identify and provide mitigations for compat breaking changes as follows:

  1. During PRs, teams should identify compatibility breaking changes and determine what are the driving factors for the change. Also determine what would be the cost to make a compatible change. These will help determine whether additional data should be gathered or whether the change will be made regardless of what the data indicates.
  2. Gather data to understand impact of the change e.g. contact Mark Miller or .NET Technical Insights NET_TI@microsoft.com
  3. Use AppContext switches so that users can opt-in or opt-out of the breaking change.
  4. At minimum, teams need to integrate API Compat tool into their build system to identify modification or deletion of APIs. Detailed scenario descriptions for API Compat and other recommended validations are listed in the Compatibility Quality Indicators section below.

Compatibility breaking change process:

  1. Forum for compatibility breaking change review and discussion:
    • For Previews, please include Mark Miller and Priya Purkayastha in PRs that discuss issues with potential compatibility impact
    • For RC and beyond, breaking change issues need to be brought in for approval to .NET Core Tactics. Teams will get an opportunity to bring their breaking changes to QB attention and for discussion.
    2.All intentional breaking changes need to be documented using the breaking change issue template and process defined

@danmoseley
Copy link
Member

@richlander I'm going to close this because we have an existing process and I think it's confusing to have this issue that's directing an alternative process. I suggest that you have an offline discussion with @PriyaPurkayastha who owns compat/breaking changes.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants