-
Notifications
You must be signed in to change notification settings - Fork 34
Changelog directive enhancements #2576
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
Conversation
|
Thanks for making these enhancements! Simple single-repo use caseI tested against the simple single-repo elasticsearch use case (using files from elastic/elasticsearch#140795) and the subsections option works there.
Multi-repo use caseI also tested against the multi-repo Cloud Serverless use case, which is more complex because:
I've drafted the test in elastic/docs-content#4843, which happily seems to support not copying the changelogs over as long as the bundles were created with The issues in the case seem to be:
|
|
Nice work! I want to discuss the options: I think its better to be prescriptive and uniformity is a feature.
|
Mpdreamz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small code nits and a question about directive properties.
| /// 2. changelog.yml in the docset root | ||
| /// 3. docs/changelog.yml relative to docset root | ||
| /// </summary> | ||
| private void LoadConfiguration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should load ChangelogConfigurationLoader we probably want to inject a singleton in IServices for the configuration like we do for other configurations.
| if (string.IsNullOrWhiteSpace(configPath)) | ||
| return; | ||
|
|
||
| PublishBlocker = ChangelogConfigurationLoader.LoadPublishBlocker(fileSystem, configPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be read from the singleton changelog config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait it can be a singleton in assembler builds but it can be shared on the relevant BuidContext. Which would make it available in markdown parsers.
| /// </summary> | ||
| /// <param name="bundles">The sorted list of bundles to merge.</param> | ||
| /// <returns>A list of bundles where same-target bundles are merged.</returns> | ||
| private static List<LoadedBundle> MergeBundlesByTarget(List<LoadedBundle> bundles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should live in Elastic.Changelog as a service.
| :hide-features: feat-1 , feat-2 , feat-3 | ||
| ::: | ||
| """) => FileSystem.AddFile("docs/changelog/bundles/9.3.0.yaml", new MockFileData( | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ultra nit add // language=yaml and // language=markdown hints to these so they highlight in supporting editors.
|
Here are my two cents with respect to Martijn's comments:
Yes, for example the links to the elasticsearch-serverless repo in elastic/docs-content#4627 test. That would be great if it could be auto-detected and auto-hidden.
This relates to the
Right now it reflects the difference between the way we handle release notes in Cloud vs Stack (e.g. no sections in https://www.elastic.co/docs/release-notes/cloud-serverless and sections in https://www.elastic.co/docs/release-notes/elasticsearch). I don't know the history of why they don't have sections in the Cloud context, so I was just matching the existing behaviour in the short-term.
IMO it's worth keeping in case we are publishing two or more release notes from a single repo. Currently that's true for the docs-content repo (though I haven't tried combining the Cloud Serverless and Observability and Security release notes into a single config file yet... maybe it's doable).
This is currently for the use case where we're releasing bundles from multiple repos on the same date (e.g. the Cloud Serverless use case tested in elastic/docs-content#4627 and elastic/docs-content#4843) |
Cool we can! 😸
++ we need to encode that information into the bundle. A bundle profile (see #2577) could list the features to hide and bake it into the bundle. That way folks can update the config prior to the bundle being created by automation. We could even extend
Ok if its really needed lets use the config for this, ideally we normalize this if folks do not have a strong opinion.
Can you tell me more why docs-content is doing multiple release-notes?
Cool, I think we should always merge and not expose this option. |
The https://www.elastic.co/docs/release-notes/observability and https://www.elastic.co/docs/release-notes/security are a subset of the Kibana release notes (split off by area) so I'm not sure why they are being stored in docs-content but even if they were stored in the kibana repo, the question will be whether they can all be reasonably generated from a single changelog.yml or whether we need to support multiple. I think it should be doable to put in a single, so I'll mock up a test. |
# Conflicts: # src/Elastic.Markdown/Myst/Directives/Changelog/ChangelogInlineRenderer.cs
…umentation and Elastic.Documentation.Configuration to disentangle from the service project.
# Conflicts: # src/Elastic.Documentation/ReleaseNotes/ChangelogTextUtilities.cs
Mpdreamz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I do think we should remove :config: path but lets follow up :)
Summary: Review Comments → Commits MappingHere's a breakdown of which commits addressed which review feedback: @Mpdreamz's Directive Options Discussion
@Mpdreamz's Code Architecture Comments
github-code-quality bot: Path.Combine Warnings
@lcawl's Testing Feedback
Additional Enhancements (New Functionality)
Outstanding Items for Follow-upPer Mpdreamz's approval comment:
The |
Summary
This PR brings the
{changelog}directive to feature parity with thechangelog renderCLI command.Key improvements:
2025-08-05):type:— Filter entries by type:subsections:— Group entries by area/component:config:— Explicit path tochangelog.ymlchangelog.ymlconfiguration for publish blockersAutomatic behaviors (no configuration needed):
New Directive Options
:::{changelog} /path/to/bundles :subsections: % Group entries by area (default: false) :config: /path/to/changelog.yml % Explicit config path :type: all % Determine the release notes content by type (default: all except known-issue/deprecation/breaking-change) :::Options
:type: value:subsections:false:config: path:type:Valuesallbreaking-changedeprecationknown-issueRefactorings
Relocated and consolidated release notes types:
Elastic.Changelog.Serialization.*Elastic.Documentation.Configuration.ReleaseNotes.*Elastic.Markdown.Myst.Directives.Changelog.LoadedBundleElastic.Documentation.ReleaseNotes.LoadedBundleReleaseNotesSerializationTest Plan
dotnet formatverification passesdotnet buildsucceeds