Skip to content

Conversation

mkArtakMSFT
Copy link
Contributor

@mkArtakMSFT mkArtakMSFT commented Mar 25, 2023

This change is inspired by the breaking change automation process used in the dotnet/runtime repository. However, these are differences between the two. Basically, this process is only to remind engineers to announce breaking changes by filing appropriate announcement issues in the aspnet/announcements repository.

Here what the automation does:
When a PR is labeled with the breaking-change label, the bot will add a needs-breaking-change-announcement label to the issue and drop a comment with instructions about what needs to be done for the announcements. Below is an example as if I've added the breaking-change label:

Thanks for identifying a breaking change.

@mkArtakMSFT, after you commit this PR please take the following actions, as part of the breaking changes announcement process:

  • Create an announcement issue by using the ASP.NET Core breaking change issue template.
  • Link the breaking change announcement issue from this PR.
  • Remove the needs-breaking-change-announcement label.

@mkArtakMSFT mkArtakMSFT added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 25, 2023
@mkArtakMSFT mkArtakMSFT requested review from a team and wtgodbe as code owners March 25, 2023 00:05
@ghost
Copy link

ghost commented Mar 25, 2023

Hey @dotnet/aspnet-build, looks like this PR is something you want to take a look at.

@danmoseley
Copy link
Member

Create an announcement issue by using the ASP.NET Core breaking change issue template.

do those end up being folded into pages like this https://learn.microsoft.com/en-us/dotnet/core/compatibility/7.0 ? or is some other action required there.

@danmoseley
Copy link
Member

One other thing -- whether by design or not, I believe the runtime folks don't gate the PR on this. The label is a marker that they use to do regular sweeps. A possible advantage of that is that breaking changes and PR's don't necessarily match neatly 1:1 so it might make sense sometimes to write the issue based on several changes.

I think the text above is not saying that the PR cannot be merged until this is done, but perhaps could be a bit more explicit about that.

@mkArtakMSFT
Copy link
Contributor Author

Create an announcement issue by using the ASP.NET Core breaking change issue template.

do those end up being folded into pages like this https://learn.microsoft.com/en-us/dotnet/core/compatibility/7.0 ? or is some other action required there.

From what I understand, and @gewarren can confirm this, people on the docs side watch for all the Breaking Change announcements in the aspnet/announcements repo, which don't have the Documented label, and document these periodically, and that's how these changes show up in the linked docs page: https://github.com/aspnet/Announcements/issues?q=is%3Aissue+is%3Aopen+label%3A%22Breaking+change%22

@mkArtakMSFT
Copy link
Contributor Author

mkArtakMSFT commented Mar 25, 2023

One other thing -- whether by design or not, I believe the runtime folks don't gate the PR on this. The label is a marker that they use to do regular sweeps. A possible advantage of that is that breaking changes and PR's don't necessarily match neatly 1:1 so it might make sense sometimes to write the issue based on several changes.

I think the text above is not saying that the PR cannot be merged until this is done, but perhaps could be a bit more explicit about that.

Correct, they don't block these PRs, neither should we. In fact, announcement, etc. should come after the change has been made. Hence my current wording: when you commit this breaking change please take the following actions. But it seems this is not clear enough. So I'll add a note to be more explicit.

See the updated text, which goes as follows: ...after you commit this PR...

@mkArtakMSFT mkArtakMSFT merged commit 7aca303 into main Mar 27, 2023
@mkArtakMSFT mkArtakMSFT deleted the mkArtakMSFT/breaking-change branch March 27, 2023 20:00
@ghost ghost added this to the 8.0-preview4 milestone Mar 27, 2023
@gewarren
Copy link
Contributor

From what I understand, and @gewarren can confirm this, people on the docs side watch for all the Breaking Change announcements in the aspnet/announcements repo, which don't have the Documented label, and document these periodically, and that's how these changes show up in the linked docs page: https://github.com/aspnet/Announcements/issues?q=is%3Aissue+is%3Aopen+label%3A%22Breaking+change%22

Yes, that's correct :)

@ghost
Copy link

ghost commented Mar 28, 2023

Hi @gewarren. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants