search

Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
109 stars 166 forks source link

[PR Workflow][SDK] Flag the spec PR of brownfield service converting (migrating) to TypeSpec from Swagger #8339

Open raych1 opened 1 month ago

raych1 commented 1 month ago

Context For brown field services of management plane, migrating to TypeSpec may introduce a few breaking changes to the SDK. We have observed that such spec PRs can be merged directly without the Shanghai SDK team's review. This is an example spec PR

This process could result in potential breaking change to the SDK haven't been evaluated, leading to additional efforts to mitigate these breaking changes post-merge.

Proposal

CC: @weshaggard @ArthurMa1978

weshaggard commented 1 month ago

Adding @mikeharder and @konrad-jamrozik to this discussion.

Are we sure having a TypeSpec-Migration label is the best way to handle this? I suspect we need to work on making the sdk generation steps more reliable and then making them required if we want to prevent these types of issues.

maririos commented 1 month ago

I am trying to understand why we need this. Is this issue suggesting that the current SDK generation checks that adds, for example, CI-BreakingChane-Go, doesn't cover management plane breaking changes?

raych1 commented 1 month ago

@weshaggard , service team can opt out the SDK generation by leaving empty for the configuration in either the readme.md file or the tsp-config.yaml file. Unless SDK generation is enforced in the spec PR, we cannot rely on SDK generation to gate the migration scenario.

@maririos , the current SDK automation check is capable of flagging breaking changes to both mgmt. plane and data plane GA SDKs. This issue is particularly relevant for brownfield services which are migrating to TypeSpec. As more and more brownfield services migrate to TypeSpec, we anticipate this issue will become increasingly important. Therefore, we would like to be involved earlier in the spec review process, particularly when service team is working on the preview API version which typically occurs in the private spec repo.

konrad-jamrozik commented 1 month ago

@raych1 did the example spec PR opt out from SDK generation? If it did, then it did not break any SDKs, yes? But the concern is that if later someone else decides to re-enable SDK generation only then we will discover there is an issue?

But if someone re-enables SDK generation at later date, won't the SDK breaking change detectors add appropriate BreakingChange-<Lang>-Sdk-* label as @maririos mentioned?

Or are you saying that TypeSpec conversion may end up regenerating SDKs with breaking changes without the checks detecting them?

If this is an issue with SDK generation breaking change checks not detecting an issue even if the SDK-generation is declared, I would like to see an issue tracking the exact problem with these checks.

If this is an issue with SDK generation being disabled when the API version is converted, but later on causing trouble because someone will enable it at later date and the SDK generation breaking change checks won't notice there is a problem, I would like to see an issue explaining and tracking this scenario.

I agree with @weshaggard - this appears to be a gap with the checks detecting SDK breaking changes that needs to be addressed. Or maybe we need some automation that will check that TypeSpec-conversion PR does not disable SDK generation, so that SDK breaking change detectors get a chance to run and report any issues.

FYI @mikeharder @MattGertz

raych1 commented 1 month ago

@konrad-jamrozik I want to clarify that the SDK breaking change detection tool will definitely gate on breaking changes to SDK. We are extending its capabilities to support both MPG and DPG scenarios.

This feature request is primarily aimed at flagging spec PRs related to TypeSpec conversion scenario. Since SDK team is not a required reviewer for ARM spec PRs, we might be bypassed during the TypeSpec conversion reviews. Therefore, we would like to proactively monitor these TypeSpec conversion spec PRs. Labelling them is one feasible approach.

@weshaggard and I aligned on the necessity of flagging the brownfield services migration and will need further discussion to finalize the timing and mechanism for this flagging.

konrad-jamrozik commented 1 month ago

@raych1

You wrote:

Since SDK team is not a required reviewer for ARM spec PRs, we might be bypassed during the TypeSpec conversion reviews.

Is your concern here that even if automation adds a label denoting that given PR has an SDK breaking change, your team won't notice this, or someone else is going to approve that label without waiting for your team?

Or is your concern here that your team wants to review these PRs even if no SDK breaking changes were detected?

raych1 commented 4 weeks ago

@konrad-jamrozik Shanghai team would like to get notified for these PRs in general even if they don't have breaking change reported.