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

[Breaking Change] (Cross Version) should not compare against old preview #5024

Open
mikekistler opened this issue Dec 22, 2022 · 11 comments
Open
Assignees
Labels
Breaking Changes Improvements to Breaking Changes tooling Central-EngSys This issue is owned by the Engineering System team. feature-request This issue requires a new behavior in the product in order be resolved. Spec PR Tools Tooling that runs in azure-rest-api-specs repo.

Comments

@mikekistler
Copy link
Member

mikekistler commented Dec 22, 2022

In PR 21606 the Breaking Change(Cross Version) checks compared the new api version to the most recent stable, 2022-05-01, and to an older preview version, 2021-04-01-preview.

image

There is no point in reporting differences to a preview that is older than the most recent stable api version. This clutters the report with issues that are not relevant and potentially wastes reviewers time investigating these issues.

This issue belongs to a set of related (and possibly at least partially duplicate) issues:

@ghost ghost added the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Dec 22, 2022
@mikekistler mikekistler added Spec PR Tools Tooling that runs in azure-rest-api-specs repo. and removed needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. labels Dec 22, 2022
@raych1
Copy link
Member

raych1 commented Jan 11, 2023

@jianyexi , looks like the preview version earlier than last GA can be excluded from comparison as per the guidance https://github.com/Azure/azure-rest-api-specs/blob/main/documentation/ci-fix.md#breaking-change-check.

@jianyexi
Copy link

This is by design, as the breaking change (cross-version) will select the latest version which exists in the main branch to compare with the new version in the PR

@mikekistler
Copy link
Member Author

It looks to me like 2022-05-01 exists in the main branch and is newer than 2021-04-01-preview, which would make 2022-05-01 the latest version in the main branch. So what is the reason for comparing against 2021-04-01-preview?

@mikekistler
Copy link
Member Author

Discussed in REST API Tooling meeting. Concluded that we want to compare against the most recent GA version (if one exists). We also compare against the latest preview version but only if it is more recent than the latest GA, and these reported as warnings. This rule applies for both new GA version and new preview version.

@mikekistler mikekistler moved this to Backlog in Spec PR Tools Jan 18, 2023
@jianyexi jianyexi assigned konrad-jamrozik and unassigned jianyexi Mar 13, 2023
@heaths
Copy link
Member

heaths commented Mar 27, 2023

I'd argue it's worth reporting, but not as a required check. I've seen cases where a couple previews in flight should have both gotten fixes but didn't, and the breaking change was a good indication of that.

@konrad-jamrozik
Copy link
Contributor

@mikekistler could you clarify what is the required behavior change here? Not exactly clear to me upon reading through the discussion. Thank you!

@konrad-jamrozik konrad-jamrozik added the feature-request This issue requires a new behavior in the product in order be resolved. label Apr 6, 2023
@mikekistler
Copy link
Member Author

The desired behavior is described in this comment and we should make whatever changes are needed to make this be the actual behavior.

@konrad-jamrozik konrad-jamrozik changed the title Breaking Change(Cross Version) should not compare against old preview [Breaking Change] (Cross Version) should not compare against old preview May 12, 2023
@konrad-jamrozik
Copy link
Contributor

There exists an open PR that is supposed to potentially address this:

@konrad-jamrozik
Copy link
Contributor

Another case:

@konrad-jamrozik
Copy link
Contributor

This work item was mentioned by @mikekistler in recent discussion on comparing to RPSaaSMaster. The discsusion on Teams is here.

@konrad-jamrozik
Copy link
Contributor

konrad-jamrozik commented Apr 19, 2024

Additional context in Teams, here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Changes Improvements to Breaking Changes tooling Central-EngSys This issue is owned by the Engineering System team. feature-request This issue requires a new behavior in the product in order be resolved. Spec PR Tools Tooling that runs in azure-rest-api-specs repo.
Projects
Status: 📋 Backlog
Status: 📋 Backlog
Development

No branches or pull requests

5 participants