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

DO NOT MERGE: TEST PR. Update wayfind.json to test ParameterLocationHasChanged #3371

Closed
wants to merge 1 commit into from

Conversation

konrad-jamrozik
Copy link

@konrad-jamrozik konrad-jamrozik commented Jun 21, 2024

Copy link

openapi-pipeline-app-test bot commented Jun 21, 2024

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ This PR has at least one breaking change (label: BreakingChangeReviewRequired).
    To unblock this PR, follow the process at aka.ms/brch.
  • ❌ The required check named Swagger BreakingChange has failed. To unblock this PR, follow the process at aka.ms/brch.

Copy link

openapi-pipeline-app-test bot commented Jun 21, 2024

Swagger Generation Artifacts

️🔄[ignore this check status; not blocking PR merge] ApiDocPreview inProgress [Detail]
Posted by Swagger Pipeline | How to fix these errors?

Copy link

openapi-pipeline-app-test bot commented Jun 21, 2024

Generated ApiView

Language Package Name ApiView Link
Swagger Creator https://apiviewstagingtest.com/Assemblies/Review/2a9b4231903d447ca023dc3869e280f9?revisionId=fd480e7a3d86460185e23359033cb2ab

Copy link

openapi-pipeline-app-test bot commented Jun 21, 2024

Swagger Validation Report

️❌BreakingChange: 1 Errors, 0 Warnings failed [Detail]
Compared specs (v0.10.12) new version base version
wayfind.json 2022-09-01-preview(a8f40f5) 2022-09-01-preview(main)
Rule Message
1050 - ParameterLocationHasChanged Parameter location has changed. Name: 'api-version'. In: 'Query'. Old location is method: 'False'. New location is method: 'True'.
New: Creator/preview/2022-09-01-preview/wayfind.json#L279:9
Old: Creator/preview/2022-09-01-preview/wayfind.json#L279:9
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️⚠️LintDiff: 0 Warnings warning [Detail]
Compared specs (v2.2.2) new version base version
package-2022-09-preview package-2022-09-preview(a8f40f5) package-2022-09-preview(main)

The following errors/warnings exist before current PR submission:

Rule Message
⚠️ PageableOperation Based on the response model schema, operation 'Wayfinding_GetPath' might be pageable. Consider adding the x-ms-pageable extension.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L271
⚠️ PaginationResponse Operation might be pageable. Consider adding the x-ms-pageable extension.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L271
⚠️ ParameterDefaultNotAllowed A required parameter should not specify a default value.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L283
⚠️ ErrorResponse The error property in the error response schema should be required.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L318
⚠️ ErrorResponse Error schema should define code and message properties as required.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L318
⚠️ ErrorResponse Error response should contain a x-ms-error-code header.
Location: Creator/preview/2022-09-01-preview/wayfind.json#L318
️️✔️[ignore this check status; not blocking PR merge] Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️[ignore this check status; not blocking PR merge] SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️PR Summary succeeded [Detail] [Expand]
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

@konrad-jamrozik
Copy link
Author

@konrad-jamrozik
Copy link
Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@konrad-jamrozik
Copy link
Author

konrad-jamrozik commented Jun 21, 2024

This should have reported failure instead of

detectBreakingChange: Prevented spurious failure of breaking change check. prKey: test-repo-billy/azure-rest-api-specs/3371, comparisonType: SameVersion, oadViolationsCnt: 1, process.exitCode: 0.

?

https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3895887&view=logs&j=98f87804-8e1f-5655-af02-e80aefa7aa97&t=cc3a9482-d623-554e-0ea4-1b9ff4f5608e

That "spuriosity" check is there since:

See also:

@konrad-jamrozik
Copy link
Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@konrad-jamrozik
Copy link
Author

konrad-jamrozik commented Jun 21, 2024

@mikekistler:

looking at this PR and this result: https://github.com/test-repo-billy/azure-rest-api-specs/pull/3371/checks?check_run_id=26540401459

according to this diagram:
https://eng.ms/docs/products/azure-developer-experience/design/specs-pr-guides/pr-brch-deep#diagram-explaining-breaking-changes-and-versioning-issues

we are in the "same-version check / preview / breaking change" so automation should add VersioningReviewRequired label. Is my analysis correct?

@mikekistler
Copy link

Agreed. Should have VersioningReviewRequired.

@konrad-jamrozik
Copy link
Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@konrad-jamrozik
Copy link
Author

Agreed. Should have VersioningReviewRequired.

Pull Request 560333: breaking changes: Fix ParameterLocationHasChanged not triggering for "same-API" version check

@konrad-jamrozik konrad-jamrozik deleted the konrad-jamrozik-patch-8 branch July 8, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants