-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add tool to generate changelog for Azure SDK for Dotnet #6033
Conversation
@RodgeFu can you provide some elaboration on this work in the PR description? Please include link to the relevant issue and/or project. In addition, please include a Currently I can only guess what this is about. |
@konrad-jamrozik, thanks for looking at the PR. The issue is: Azure/azure-sdk-for-net#35186 following is a brief of the tool.
The tool takes the api file as argument (i.e. ChangelogGen ...\azure-sdk-for-net\sdk\compute\Azure.ResourceManager.Compute\api\Azure.ResourceManager.Compute.netstandard2.0.cs) and would merge the generated release notes into the changelog.md file like the "## 1.0.2 (2023-04-27)" release in https://github.com/RodgeFu/azure-sdk-for-net/blob/main/sdk/websites/Azure.ResourceManager.AppService/CHANGELOG.md I dont see readme file in other tools under Dotnet. Is there any template I should use for the Readme.md? Or a brief description like above is good enough? Thanks. |
@konrad-jamrozik, please help to review the PR. Feel free to let me know if there is anything else missing. thanks. |
adding @hallipr and @jsquire as @RodgeFu apologizes for the delay! I am wondering - did you had a chance to collaborate with anyone from the Redmond Azure SDK EngSys team in preparation for making this PR? For example, @hallipr or @weshaggard ? Just trying to figure out if there is anybody on my team that has additional context for this work and as such should be aware of this PR. Re this:
I think if you copy-paste your PR comment into the README with slight adjustments to make the context more appropriate, this would work. |
@konrad-jamrozik, thanks for the reply. I haven't collaborated with anyone on your team for the work item. Include @ArthurMa1978 in case there is more background before the work item is assigned to me. As for the README, i have added a readme.md to the PR. feel free to let me know if more adjustments are needed. thanks. |
I haven't had a chance to review yet, but I'd like to understand the goals and intended use of this before we merge the PR. I'd also like to request that we add meaningful descriptions to pull requests when opening them. Someone unfamiliar should be able to look at a PR and understand the context. //cc: @ArthurMa1978, @RodgeFu |
@RodgeFu I'd like to better understand to goals of this tool. In general, some of the other languages have generated changelogs with these library diffs and I'd say for the most part they have contained more noise than value for the consumers of the changelog. So much so that we have filtered them out in our release notes and release blog posts. |
@weshaggard, the goal of this tool is to generate the common part in changelog so that people don't need to do it manually every time including list the change in API interface, upgrade in the dependency version of Swagger, Azure.Core and Azure.ResourceManager now. The information are needed for each release and have to be checked and written manually every time now. So having a tool to generate them automatically will be helpful to save the dup manual cost as well as avoid possible mistakes when writing them manually. I am not sure about the noise you mentioned in the changelog in other language. It will be helpful if you can provide more detail or example about the noise so that we can see whether it also applies here. thanks. Agree to below and will make sure more description will be added next time. Furthermore, I am interested in whether there is any recommended way that we can contact your team first when we want to add more tools to help our work in Azure SDK so that we may collaborate or leverage your team's knowledge/resource in some way and benefit both sides. thanks.
|
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.
Flagging to prevent accidental merges until I have a chance to review and we understand the intended scenario and usage for this tool.
@RodgeFu: Are you intending this to be something manually run by developers to populate the change log that you'll manually review/trim or are you intending this to be integrated with CI? Generally speaking, we do NOT want to list out every single API change in the changelog - that's not something that is contextually helpful for customers and creates a lot of noise that hides information that they actually need. There are some exceptional cases (compilation breaking changes, specifically) where we do want to call out the exact member and point customers at a file that shows migration examples, but not for every addition/change and especially not for preview releases which we expect to have churn. The goal of a change log is to highlight for customers changes that may impact them or they may wish to take advantage of in the context for how they would use the SDK. We want to curate this and summarize at a high level, not provide an API diff. Please read the Azure SDK policy for release notes - our change logs may go into a bit deeper detail than the release notes, but should generally follow this guidance. To highlight the point that Wes is making, consider the following contrived examples: Poor change log5.10.0-beta.1 (Unreleased)Features Added
Breaking Changes
Bugs Fixed
Other Changes
Good change log5.10.0-beta.1 (Unreleased)Features Added
Bugs Fixed
|
src/dotnet/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.Tests/TestApiComparer.cs
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.csproj
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.csproj
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.csproj
Outdated
Show resolved
Hide resolved
src/dotnet/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/ChangeLogResult.cs
Outdated
Show resolved
Hide resolved
Looking through the code, I'm not comfortable with this automatically generating change logs. If we're going to use it as a diff tool to make authors aware of what changed so that they can craft a good quality change log, we should output to something that is obviously temporary. |
As an example, take a look at https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/network/azure-mgmt-network/CHANGELOG.md#2100-2022-08-05 which is automatically generated but we end up filtering this out when we publish the release notes at https://azure.github.io/azure-sdk/releases/2022-08/python.html. That is because it flooded the entire notes and we couldn't find the more interesting changes. I've found that a blind api diff in changelogs doesn't end up adding a lot of value. |
This tool is expected to be triggered manually and the generated and merged changelog will be reviewed through standard PR review process which should be able to make sure they are reviewed properly before check-in. |
If we're talking about a GA package, then I agree. If we're talking changes from a beta, then no. We market the Azure SDK as having a very strong focus on backwards compatibility and NOT making breaking changes to GA packages. We are clear that the same guarantees do not apply to betas and that beta packages should not be used in production applications. We generally want to avoid having change logs that list API tweaks that are breaking for betas. It makes it more difficult for customers to find breaking changes to GA versions that are impactful for production applications by introducing a lot of visual noise. It also presents poor optics, as a quick scan of a change log with a large list of breaking changes communicates that these are not, in fact, rare occurrences and weakens perceptions of our guarantees. //cc: @czubair and @KrzysztofCwalina for additional perspectives. |
This tool can assist us to capture all changes from a new release.
|
Yes, the phrase "breaking change" is incorrect because we always fix them. |
I agree with @jsquire that change logs should not include automatically generated list of every API change. As per these carefully thought-out guidelines (Azure SDK policy for release notes)- they should be limited to developer impacting changes. Customers do not need to know about an API change unless there is an action they need to take or it impacts a functionality they may already be using. |
If we wanted to provide a listing of all API changes, I think we would need a better format for that. Maybe something like our API view diff. I totally agree with @jsquire and @weshaggard that the format in this PR is more of a noise than helpful. |
Thanks for all the inputs. So as a summary, instead of listing all the API changes, we will only have following info generated in the changelog (assume we won't have any breaking change in API)
These notes will be under the 'Other Changes' section. I will update the code accordingly if there is no more comment. thanks. |
For GA versions only, please. We don't want to track beta-only breaking changes and obsoleted members. |
Yes. The obsoleted changes will only be generated for GA version. thanks. |
2. Change merge mode from group to line 3. for GA release, the previous GA release will be used as baseline
I have updated the code and comments. Please help to have a look again and let me know if there are more comments. thanks. |
tools/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.csproj
Outdated
Show resolved
Hide resolved
|
||
if (ApiChange?.GetBreakingChanges().Count() > 0) | ||
{ | ||
ReleaseNoteGroup breakingGroup = new ReleaseNoteGroup("Breaking Changes"); |
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.
What is the plan for putting together information in a breaking-changes.txt
file that demonstrate how to migrate code?
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.
Not quite sure I understand the comment correct here. Do you mean we will have a doc to demonstrate how to mitigate breaking change?
This part code is to handle the case when breaking change detected just in case though we dont expect any breaking change. In case there are breaking changes, the tool will log an error as well as adding these breaking change into changelog under "Breaking Changes" section anyway to make sure people are aware of these unexpected breaking changes and fix them when reviewing PR.
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.
Do you mean we will have a doc to demonstrate how to mitigate breaking change?
Yes. That is the standard for Azure SDK packages. [Example]. Here, again, I don't know if there was an exception granted to management libraries. @ArthurMa1978?
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.
Once the management plane SDK reaches GA, no new releases will introduce breaking changes. Any breaking changes in the swagger spec will be handled by using customization code (or autogen code in the future).
So just skip this part here.
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.
@jsquire , kindly ping..., please let us know if you have further questions about this. thanks.
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.
Once the management plane SDK reaches GA, no new releases will introduce breaking changes. Any breaking changes in the swagger spec will be handled by using customization code (or autogen code in the future).
So just skip this part here.
We generally do not document API breaking changes from beta-to-beta. I thought we agreed to only list these for GA libraries - so if this section does not apply to GA libraries, then let's not include it or, let's at least have some minimal plan for how we'll communicate to partner teams that they need to put together a breaking changes guide.
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.
I misunderstood that we will fix breaking change for beta too which seems not true. fixed to ignore breaking changes for beta version. Thanks.
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.
Can you help me understand where that logic is? I don't see a filter for breaking changes or obsolete against beta versions.
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.
the original logic was when calling GenerateReleaseNote method in ChangeLogResult.cs, it took parameters 'ignoreBreakingChange' and 'filter'. the 'ignoreBreakingChange' would be true for preview release to skip them while only ObsoleteChange would be set in filter to only log obsoleted changes. Just now, I realize that the api comparison part is just not needed at all for preview version, so i have updated the code to skip that part for preview version to make the logic more clean and bring better perf and stability. thx.
tools/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/ChangeLogResult.cs
Outdated
Show resolved
Hide resolved
tools/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/ChangeLogResult.cs
Outdated
Show resolved
Hide resolved
tools/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen/Compare/ApiComparer.cs
Outdated
Show resolved
Hide resolved
/azp run tools - Azure.SDK.ChangelogGen - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run tools - Azure.SDK.ChangelogGen - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
tools/Azure.SDK.ChangelogGen/Azure.SDK.ChangelogGen.Tests/Azure.SDK.ChangelogGen.Tests.csproj
Outdated
Show resolved
Hide resolved
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.
Let's please rename this project to include a reference to "management" or "mgmt" or "resource manager"
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.
renamed to Azure.SDK.Management...
} | ||
|
||
public abstract string Description { get; } | ||
public virtual bool IsBreakingChange |
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 isn't the only thing that can cause a breaking change. I am concerned that we're oversimplifying this check and other breaking changes will be overlooked because people assume this tool is doing deeper analysis than it is.
As a couple of examples:
- The order of parameters was changed
- A parameter was renamed
- A new overload was added with a subset of existing parameters and different behavior
- A default value was changed which changes behavior
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.
A breaking change means an original interface no longer available (removed) in the end. all the examples you listed above will trigger two changes detected, one new interface added and one old interface removed. and all removed interfaces will be treated as breaking change.
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.
The other examples are also breaking changes and will need to be documented and treated that way. As mentioned, my fear is that people will rely on this generator to identify breaking changes for them and miss those that are not "we removed a thing."
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.
the breaking changes from other examples will also be detected by the tool because the original interface no longer exists after the change. For example, if Method(string a, int b) is changed to Method(int b, string a). the tool will find the Method (string a, int b) can't be found in new version (removed) and log a breaking change for it.
If we think a breaking change in a general way, it's just because an interface no longer available in the new version (treated as removed by the tool) no matter it's because the interface is deleted or renamed or modified to a different signature...
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.
That's fair. I overlooked that the signature comparison would catch the two structural cases:
- The order of parameters was changed
- A parameter was renamed
The other two cases would require semantic analysis to catch. Since we're scoping this to just management libraries and Arthur already signed-off, I won't block on this but I do still want to register my concerns about it - particularly for partner teams.
tools/changelog-gen-for-net/Azure.SDK.ChangelogGen/ChangeLogResult.cs
Outdated
Show resolved
Hide resolved
2. improve log 3. support same version of last release as the given one to improve the re-run user experience
/azp run tools - net-changelog-gen-mgmt - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
I updated the pipelines with the new location of the ci.yml based on the file path change. |
2. fix a bug in method default value check
/azp run tools - net-changelog-gen-mgmt - ci |
Azure Pipelines successfully started running 1 pipeline(s). |
This is a tool to update changelog.md with generated release notes for Azure SDK for Dotnet.
In detail, following release notes will be generated automatically:
Usage:
Example:
Remark: