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

tools: add Breaking Change issue template #23326

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

tools: add Breaking Change issue template #23326

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d98705a
tools: add Breaking Change issue template
jason-ha Dec 13, 2024
32e2ee1
tools: update version pattern in issue title
jason-ha Dec 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/ISSUE_TEMPLATE/breaking_change.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
---
name: Breaking Change
Copy link
Contributor

Choose a reason for hiding this comment

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

Does yaml front-matter in Markdown support yaml comments? If so, probably worth documenting these properties.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea. I just copied exiting template and edited.
Example at https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/syntax-for-issue-forms doesn't have comments.
I also don't know how to test this before putting it out there - docs suggest there isn't.

about: Track plans for a breaking change
title: "Removal of ? in v2."
jason-ha marked this conversation as resolved.
Show resolved Hide resolved
labels: "breaking change"
assignees: ""
---

## Broken API or pattern

<!-- Name the APIs involved specifically, etc. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommendation for format here? Commented-out examples for these sections (expected to be removed) might be useful.


**Preparation References**

<!-- A URL to the deprecation release notes. Alternatively, describe actions required where broken API or pattern is in use. -->

**Packages**
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems duplicative of what will be in the change set for the break itself

Copy link
Contributor

Choose a reason for hiding this comment

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

can we reuse that changeset for much of this, it feels quite redundant to repeat the same things in different places and formats

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 release notes URL is the way to avoid redundancy. There should already be a changeset from deprecation changes that have made it to a release. ...unless you are really pushing the envelope. In the envelope pushing case, it would be okay to edit the issue after release or copy the content from the changeset.

Copy link
Member

Choose a reason for hiding this comment

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

+1 on this section in particular - typing out the list of packages feels very tedious, when it's already captured in the changeset.

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 think I misread the initial comment. I don't know how to get GitHub to reference a single line. I focused on the deprecation notes section, but I suspect this is for packages.
Packages is here because we want a search hit and we want easy collection/survey of impacted packages.
Remember this is a service we are providing to customers. (Also, part of a commitment for the legacy break process.)
The impacted packages should normally be one or two. Doesn't seem like a big hit.
Alternatively, if all of the information is readily gatherable, then perhaps write script to generate this.

Copy link
Member

Choose a reason for hiding this comment

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

this is a service we are providing to customers

I see 3 different information sources here:

  1. The stream of past release notes covering a variety of "upcoming" breaks (deprecations, etc)
  2. These sub-issues
  3. The release notes for the 2.x0 release covering all the breaks.

Stepping back to discuss the value prop of (2) as a whole -- During what time period would a consumer review this issue, and learn something new or do something different, compared to existing workflows around (1)? In other words, I expect that a highly engaged partner is reading all release notes for every minor, and taking action to prepare for the future. In that case, this issue will be a checksum, ideally a no-op. Same with (3).

So the value prop seems to be -- we're putting in extra work to distill and refactor (1) as a way to peer into the future and see what (3) will be - while there's still time to do something about it without delaying the bump.

Doesn't seem like a big hit.

I see that, but I also feel the resistance to it. When do I ever type a package name? I don't - Imports happen automatically, the changeset tool gives me a list to choose from, etc.

Hopefully that helps clarify what I'm seeing / bringing to this, for whatever it's worth.

if all of the information is readily gatherable, then perhaps write script to generate this.

Not sure how this would be accomplished without updatine changeset semantics to include the future version where a break will happen (in the changeset announcing it). But then what if plans change? Seems tricky.

Copy link
Contributor Author

@jason-ha jason-ha Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(@markfields - so that maybe you get a notification)
Probably less useful for the very active consumers, but I am not sure how many of those we have. There are a lot deprecated API uses in our codebase and some consumers. There are a number of things get deprecated with an expected timeframe and we slip far past that. In the 2.20 round there is at least one very old deprecation.
So, these (2) make up the we-are-serious notifications and are probably the most important of the streams. The third stream being the release where the breaks happen is too late for our purposes.
I think you might be underestimating how lazy because we are busy software engineers are.

I feel the resistance. It is hard to measure the needs of customers until we do something more uniform. It is also hard to see how hard it is to handle this request without trying. I feel like per the prior section the release notes / change set should be on-hand and you just need a little copy-paste.

Part of the point of suggesting automation is that if it is hard to figure out, then (assuming the information is useful in the first place) it we should make it easy.

Is the information useful?

When I see a random API name, I don't know what package it is from often. As a consumer with API name and package name I can make enough links in my brain to have a really good guess if my stuff is impacted. Packages are the high-order bit and could make sense to stick in the title, but that is just too much. I debated placing Packages before Preparation References, but I felt like people who didn't recognize the API could scroll a bit and those who do and will have work to do get to the stuff they want faster (shorten the longer pole).

Copy link
Member

Choose a reason for hiding this comment

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

Definitely willing to try it! As long as we are collecting feedback from those who use this workflow (or that don't!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - I don't think I can get the feedback until 2.30+ since there is a lot of variety in 2.20 without this simplified template in place.


<!-- A simple list of impacted packages -->

**Expected Timeline**
Copy link
Member

Choose a reason for hiding this comment

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

This is implicit?

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 is probably the most annoying block for us to fill out. But should be good for clarity to reader. Might come to issue directly and not see the parent issue (or one wasn't linked) or maybe title doesn't have it. But never implicit.


<!-- When you expect this API/pattern will be fully removed/unsupported. If possible, give a release version. -->

**Reminders**

<!-- Reminder: Assign this issue to somebody, if you are unsure who, assign it to yourself. -->

<!-- Associate PR: link a PR from test/... branch in this repo (not in a fork). -->
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for the PR from test/ in upstream and not from a fork?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we can run our integration pipelines. For the legacy-alpha breaks we'll want to run Loop integration for sure. So far that means use a branch under test/ in the microsoft/FluidFramework repository.

Copy link
Member

Choose a reason for hiding this comment

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

I get the point, but I bet this is going to be a spot where people continually miss it. I have spent time thinking through these steps a lot recently, and I still (due to muscle memory) created my branch in my fork the first time.

An alternative workflow is to push to a test branch ad hoc when it's time to run the pipeline, like git push upstream my-branch:test/foo. That's easy enough to do and document.

Also, who do you expect to be running the CI pipeline? Each breaking change author? I haven't seen that called out as a requirement or recommendation.

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 is not the only instructions. I have process pending for the wiki, but I want to say use the breaking change template; so, I need this first. This is more or a reminder because yes people will miss things.


<!-- By filing an Issue, you are expected to comply with the Code of Conduct: https://github.com/microsoft/FluidFramework/blob/main/CODE_OF_CONDUCT.md -->

<!-- Lastly, be sure to preview your issue before saving, then remove this section. Thanks! -->
Loading