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

Release notes proposal #16377

Merged
merged 28 commits into from
Dec 14, 2023
Merged

Release notes proposal #16377

merged 28 commits into from
Dec 14, 2023

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Dec 4, 2023

Hi all,

I'm very excited we will start with having release notes in this repository.
And, with some minor tweaks, I believe we can really bring it home!

Right now, the release notes don't really work with the compiler documentation website.

In this pull request, I'm suggesting a solution that tackles this issue and other related points. My proposal is straightforward and admittedly biased from my past experiences in open-source projects. It's simply a different perspective, and I'll justify the changes I've made.

Proposal:

image

Old proposal

- I added `frontmatter` to the changelog files. This makes them appear nicely on the documentation website. - I moved away from version `8.0.200.md` files towards a single changelog file. In my experience, this is more common and allows you to easily view the history of changes. I think this is slightly more convenient to trace down a potential regression when bumping multiple `FCS` versions. It is also useful for people to have a single bookmark in their browser. - In general, I really like https://keepachangelog.com/en/1.1.0/. You know what is up in a human-readable way. Splitting things into sections encourages you to give more thought to your entry and it also helps readers. - I strongly encourage we add dates to the released versions and use `Unreleased` to indicate what isn't available on NuGet today. - I proposed a new format for the individual changelog entry. I described it in `About.md`. The aim is to make things more consistent again. It may seem a bit stricter, although, over time enough examples will make it easy to stick to the same format.

Current proposal:

This might appear that I'm raising the barrier a bit for people writing the changelog entry, although, it really remains writing a one-liner. And it will heavily improve the reading side of things.

If there are requirements due to the bigger dotnet release process that play against this proposal, I'm happy to adjust to these.

I seem to value this change, although it is all very subjective, maybe people can express their thoughts on it with some emojis:

  • 👍 This would improve things.
  • 👎 The changelog is fine as is.

I'm happy to drop this if it turns out a large majority doesn't care.

Open questions / todos:

  • As the format changed, do I need to update anything for NuGet pack?
  • The pull request template should be updated.

Tagging some potential changelog readers (in random order) to get some reactions:
@SchlenkR, @pbiggar, @StachuDotNet, @isaacabraham, @nhirschey, @kMutagene, @auduchinok, @DedSec256, @dotnet/fsharp-team-msft, @Smaug123, @safesparrow, @MangelMaxime, @ncave
, @TheAngryByrd, @baronfel, @AngelMunoz, @edgarfgp, @dawedawe, @cartermp

@nojaf nojaf requested a review from a team as a code owner December 4, 2023 15:08
docs/release-notes/About.md Outdated Show resolved Hide resolved
docs/release-notes/About.md Outdated Show resolved Hide resolved
@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 4, 2023

How will version change be enforced? Say if we to freeze 17.9 branch tomorrow, and bump version to .0.300, how will we make sure section is added to respective file?

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 4, 2023

Also, who will be responsible for moving everything from Unreleased to new version (it's usually a non-issue with separate files, since it'll always check a very specific file with current version.

docs/release-notes/FSharp.Compiler.Service.md Outdated Show resolved Hide resolved
docs/release-notes/Language.md Outdated Show resolved Hide resolved
docs/release-notes/Language.md Outdated Show resolved Hide resolved
docs/release-notes/About.md Outdated Show resolved Hide resolved
@pbiggar
Copy link

pbiggar commented Dec 4, 2023

Seems good. In my experience, when contributors writes these comments, they often do not have the context to know how to explain/position their change to users. I would suggest that you set the expectation that changelog entries are able/likely to be rewritten by maintainers, either by pushing a commit in the PR, or in a later pass.

@vzarytovskii
Copy link
Member

I would suggest that you set the expectation that changelog entries are able/likely to be rewritten by maintainers, either by pushing a commit in the PR, or in a later pass.

That is precisely what this step tries to avoid. Notes are best written by the author (who knows the best what they're doing). We will unlikely to rewrite majority of them.

@pbiggar
Copy link

pbiggar commented Dec 4, 2023

I would suggest that you set the expectation that changelog entries are able/likely to be rewritten by maintainers, either by pushing a commit in the PR, or in a later pass.

That is precisely what this step tries to avoid. Notes are best written by the author (who knows the best what they're doing). We will unlikely to rewrite majority of them.

For sure! But it's useful to be able to (and set expectations that you might), even if you choose not to 99% of the time.

As an example, suppose you have 45 PRs from 6 different authors all focusing on different parts of a single feature. Obviously, it would be valuable to refactor them into 1-3 more understandable consolidated entries. You don't want to feel that you can't edit these because contributors could be annoyed that you're messing with their contribution credit.

@vzarytovskii
Copy link
Member

Also, who will be responsible for moving everything from Unreleased to new version (it's usually a non-issue with separate files, since it'll always check a very specific file with current version.

On the second thought, this might be a problem with nuget packages (we don't own release process). When we insert, the last insertion to the SDK/vs needs to be after version is changed from "unreleased" to $current, we don't always know which one is the last one to be inserted (so it can happen that release notes in nuget will be pointing to "unreleased" if we to point it via permalink or copying the whole file to it).

So at the very least I would keep it as 43.8.200 (current one) without date, and not "unreleased".

@vzarytovskii
Copy link
Member

I strongly encourage we add dates to the released versions and use Unreleased to indicate what isn't available on NuGet today.

That's not always true. Beta versions are available on nuget, and nightlies on azdo feed (not sure if anyone uses those though).

@dawedawe
Copy link
Contributor

dawedawe commented Dec 5, 2023

That's not always true. Beta versions are available on nuget, and nightlies on azdo feed (not sure if anyone uses those though).

The nightlies are used.
For example, FSAC has a nightly branch that uses the FCS nightlies to catch regressions and to adjust to FCS API changes.

@T-Gro
Copy link
Member

T-Gro commented Dec 5, 2023

I would also like to understand the process for a scenario for porting a bugfix to an already released version, and also the scenario of targeting an older release branch directly.

Who/where/when writes the descriptions in order to have mappable info for readers ? (mappable - "If I update to SDK 8.300., what bugfixes am I getting")

Copy link
Member

@T-Gro T-Gro left a comment

Choose a reason for hiding this comment

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

When it comes to the design and format proposal, this looks great. Nicely done!
I think the formatting part of this proposal could be merged ASAP just to start collecting data in a better shape.

The second aspect of this PR is orthogonal and that is the file structure.
Some time ago, I shared this view of having a single large log. And thinking that some "magic" would split them and maintain them later. But for practical purposes, I started to appreciate an automated check for correct mapping between an entry and it's target versions - not only because mistakes happen, but also because final integration time is unpredictable and so is the target version. Having this mapping derived automatically from Versions.props checks for this, as the CI check will be re-run when main is merged into a PR. The same applies for backporting into an older version for example.

A "view" (UI, email newsletter, ...) can then always merge many source files and even release branches into a single one for reading purposes and rearrange them as needed, but the opposite is not true.
The same principle like with data normalization applies - it is easier to concat data together than having to parse it and split it.

For that reason, I think a clearer process of the mapping maintenance is needed for this PR.
It does not have to be as automatic, but should in place even when to be done by humans - who writes it, when, where.
(the temporal aspect of the mapping is the tricky part here - time of writing it <> time of merging it to main <> time of making it part of SDK...)

@nojaf
Copy link
Contributor Author

nojaf commented Dec 5, 2023

How will version change be enforced? Say if we to freeze 17.9 branch tomorrow, and bump version to .0.300, how will we make sure section is added to respective file?

The first thing that comes to mind would be that the person who bumps the Versions.props file will also update the changelogs.

Move Unreleased to the next state. I'm guessing there might be a few state changes.

Where ## 43.8.100 - Unreleased goes to ## 43.8.100 - Frozen to finally ## 43.8.100 - 2024-02-04.

Also, who will be responsible for moving everything from Unreleased to new version (it's usually a non-issue with separate files, since it'll always check a very specific file with current version.

This feels like a code owner's responsibility.
It would happen I guess at the same time Versions.props is bumped.

@vzarytovskii
Copy link
Member

Yeah, I have pretty much the same thoughts as Tomáš summarized.

I really like the new format (despite me being grumpy about it before and thinking that people won't maintain it), but I went with separate files per version before, since it's easily can be automated by CI and we will be guaranteed correct version every time without having a manual step in branching process (since we now have a CI step which ensures we bump version if new one released to nuget).

I am also thinking if we can have separate files somehow, since it will be super easy to see what's new in specific version, but have a release step which will merge it to one big note?

@vzarytovskii
Copy link
Member

This feels like a code owner's responsibility. It would happen I guess at the same time Versions.props is bumped.

This then needs to be added to INTERNAL.md then.

I see one problem with that however. All PRs in progress have chance of getting it wrong (either merging it with new version or unreleased, which is also the reason I went with separate files, so it's going to enforce the check).

@nojaf
Copy link
Contributor Author

nojaf commented Dec 5, 2023

Thanks for the insight @vzarytovskii and @T-Gro!
Much appreciated. I'll explore the option of composing a page for all releases based on the individual version files. It does sound less disruptive and more practical.

@T-Gro
Copy link
Member

T-Gro commented Dec 5, 2023

I see one problem with that however. All PRs in progress have chance of getting it wrong (either merging it with new version or unreleased, which is also the reason I went with separate files, so it's going to enforce the check).

Not just that, but also not detected by git's conflict mechanism nor by a CI routine.
(one person editing Version.props, another person adding a new feature - at time of raising the two PRs, you cannot tell in advance in which order they get merged. First one goes in as-is, second one just pushes "Update branch".)

nojaf added 2 commits December 5, 2023 14:58
Move files to excluded docs folder.
Compose single release file for FCS.
@T-Gro
Copy link
Member

T-Gro commented Dec 6, 2023

Unrelated to this PR, but @vzarytovskii : check_release_notes.yml was modified, but CI check was skipped.

Is that correct behavior to enable changing the yaml without running full CI?

@vzarytovskii
Copy link
Member

Unrelated to this PR, but @vzarytovskii : check_release_notes.yml was modified, but CI check was skipped.

Is that correct behavior to enable changing the yaml without running full CI?

It's disabled globally at the moment. Probably should be enabled before merging the PR. But there's a risk that there will be permission issues again.

@vzarytovskii
Copy link
Member

Unrelated to this PR, but @vzarytovskii : check_release_notes.yml was modified, but CI check was skipped.
Is that correct behavior to enable changing the yaml without running full CI?

It's disabled globally at the moment. Probably should be enabled before merging the PR. But there's a risk that there will be permission issues again.

That said, if there are any permission issues, I will try to take care of them. @nojaf would you mind if I will be committing changes directly to this branch (if it will be necessary)?

@nojaf
Copy link
Contributor Author

nojaf commented Dec 6, 2023

would you mind if I will be committing changes directly to this branch

@vzarytovskii, not at all! But I'm happy you are asking, I won't do any rebasing in that case.

Could you perhaps also do some short writing on the release process in the About.md page?

@nojaf
Copy link
Contributor Author

nojaf commented Dec 8, 2023

@brianrourkeboll I'm proposing to bundle your parentheses PRs into a single item.

@vzarytovskii
Copy link
Member

Alright, having some issues with permissions it seems. Going to dig in more on weekend (also, will have to turn this CI step for everyone, will force merge it if it will be blocking anyone).

Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Overall good stuff. This looks decent and aligned with some other open source projects.

A small ask - please update the PR desc to match the most recent shape of the changes (after discussions). I am getting a bit lost - as in, why the PR says we are getting rid of multiple changelog files but adds new multiple changelog files and so on...

docs/release-notes/About.md Outdated Show resolved Hide resolved
docs/release-notes/About.md Show resolved Hide resolved
* `while!` ([Language suggestion #1038](https://github.com/fsharp/fslang-suggestions/issues/1038), [PR #14238](https://github.com/dotnet/fsharp/pull/14238))
```

* Choose the right section for your type of change. (`## Added`, `## Changed`, `## Deprecated`, `## Removed`, `## Fixed` or `## Security`).
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want a separate "Security" area, they fit under "Fixes". Also since we categorizing changes, it would be great to somehow mark breaking changes (or rather ask people to mark them (Breaking prefix?)

Copy link
Member

Choose a reason for hiding this comment

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

Breaking is a good idea, although AFAIU we don't really have a definition for breaking changes yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I'm on the fence with the breaking prefix. Could you give me an example of something that changed, wasn't a fix and isn't breaking? I don't think that combo happens often. If it is listed under change, you can typically assume the change is breaking.

Copy link
Member

@vzarytovskii vzarytovskii Dec 11, 2023

Choose a reason for hiding this comment

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

Breaking is a good idea, although AFAIU we don't really have a definition for breaking changes yet.

We do (always had really):

  • Any binary change in API is a breaking change.
  • Any new Obsoleted API is a breaking change.
  • Any new warning on by default is a breaking change.
  • Almost any new change in whitespace parsing is a breaking change.
  • Any change in runtime behaviour is a breaking change (if it's not an implementation detail).

In other words, for libraries - if it doesn't compile anymore with no code changes, it's a breaking change. For compiler - if it's producing any new warnings - it's a breaking change. For FSharp.Core - if it produces different values in runtime than before - it's a breaking change.

Copy link
Member

Choose a reason for hiding this comment

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

Nice summary. Worth putting somewhere maybe.

@vzarytovskii
Copy link
Member

vzarytovskii commented Dec 11, 2023

General question, @nojaf I can include markdownlint as part of check, do you think this would make sense? This would force the style to be correct, but might add some headache for contributors.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 11, 2023

Yes, I don't think that is a bad idea. People will struggle with getting the exact format right.
It may not seem that difficult, but contributors will manage to eventually do things slightly different than the 100 other examples they had 😅.

Having a tool that points that out to you is often less frustrating than when a maintainer does it.

@vzarytovskii
Copy link
Member

Update: still fighting actions' permissions. Now waiting on engineering to respond.

@nojaf
Copy link
Contributor Author

nojaf commented Dec 14, 2023

@vzarytovskii could we perhaps merge this PR without the GitHub Actions stuff?

I feel like the momentum is dropping a bit here. In other PRs, I'm hesitant to include release notes as we are changing the format there. So, I think I would prefer to take this one in, as is, and revisit the enforcing stuff later.

I'd be happy to send a subsequent PR with some missing entries.

@vzarytovskii
Copy link
Member

@vzarytovskii could we perhaps merge this PR without the GitHub Actions stuff?

I feel like the momentum is dropping a bit here. In other PRs, I'm hesitant to include release notes as we are changing the format there. So, I think I would prefer to take this one in, as is, and revisit the enforcing stuff later.

I'd be happy to send a subsequent PR with some missing entries.

Yeah, probably makes sense.

@vzarytovskii vzarytovskii merged commit 68fef88 into dotnet:main Dec 14, 2023
2 checks passed
OwnageIsMagic added a commit to OwnageIsMagic/fsharp that referenced this pull request Dec 21, 2023
* upstream/main: (166 commits)
  typo in foldBack summary (dotnet#16453)
  Fix for dotnet#83 (improve constraint error message) (dotnet#16304)
  Name resolution: resolve interfaces in expressions (dotnet#15660)
  AddExplicitReturnType refactoring (dotnet#16077)
  Disabling 2 tests: running for too long, causing CI timeouts
  Improve value restriction error message dotnet#1103 (dotnet#15877)
  Parens: Keep parens for non-identical infix operator pairs with same precedence (dotnet#16372)
  More release note entries (dotnet#16438)
  Using Ordinal is both faster and more correct as our intent is to do … (dotnet#16439)
  merge (dotnet#16427)
  Optimize empty string compares (dotnet#16435)
  Checker: recover on unresolved type in 'inherit' member (dotnet#16429)
  Release notes proposal (dotnet#16377)
  [main] Update dependencies from dotnet/source-build-reference-packages (dotnet#16411)
  Allow usage of [<TailCall>] with older FSharp.Core package versions (dotnet#16373)
  Parser: recover on unfinished 'as' patterns (dotnet#16404)
  Parens: Keep parens in method calls in dot-lambdas (dotnet#16395)
  Checker: check unfinished obj expression inside computations (dotnet#16413)
  Added default dotnet-tools + additional tasks to launch them (dotnet#16409)
  make `remarks` and `returns` visible in quick info (dotnet#16417)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants