azure-rest-api-specs icon indicating copy to clipboard operation
azure-rest-api-specs copied to clipboard

Adding a sample TypeSpec ARM project created from template for Out-of-box Green CI

Open allenjzhang opened this issue 1 year ago • 7 comments

ARM (Control Plane) API Specification Update Pull Request

[!TIP] Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

[!NOTE] As of January 2024 there is no PR assignee. This is expected. See https://aka.ms/azsdk/pr-arm-review.

PR review workflow diagram

Please understand this diagram before proceeding. It explains how to get your PR approved & merged.

diagram

Click here to see the details of Step 1

Breaking changes review (Diagram Step 1)

If the automation determines you have breaking changes, i.e. Step 1 from the diagram applies to you, you must follow the breaking changes process.
IMPORTANT This applies even if:

  • The tool fails while it shouldn't, e.g. due to runtime exception, or incorrect detection of breaking changes.
  • You believe there is no need for you to request breaking change approval, for any reason. Such claims must be reviewed, and the process is the same.
Click here to see the details of Step 2

ARM API changes review (Diagram Step 2)

  • For help with Step 2 / "ARM review", see https://aka.ms/azsdk/pr-arm-review.
Click here to see the diagram footnotes

Diagram footnotes

[1] See ARM review queue (for PR merge queues, see [2]).
[2] public repo merge queue, private repo merge queue (for ARM review queue, [1])
The ARM reviewer on-call engineer visits the merge queue twice a day, so the approximate ETA for merges is 12 - 24 hours.

Purpose of this PR

What's the purpose of this PR? Check the specific option that applies. This is mandatory!

  • [ ] New resource provider.
  • [ ] New API version for an existing resource provider. (If API spec is not defined in TypeSpec, the PR should have been generated using OpenAPI Hub).
  • [ ] Update existing version for a new feature. (This is applicable only when you are revising a private preview API version.)
  • [ ] Update existing version to fix OpenAPI spec quality issues in S360.
  • [x] Other, please clarify:
    • Sample TypeSpec generated ARM project

Due diligence checklist

To merge this PR, you must go through the following checklist and confirm you understood and followed the instructions by checking all the boxes:

  • [x] I confirm this PR is modifying Azure Resource Manager (ARM) related specifications, and not data plane related specifications.
  • [x] I have reviewed following Resource Provider guidelines, including ARM resource provider contract and REST guidelines (estimated time: 4 hours).
    I understand this is required before I can proceed to the diagram Step 2, "ARM API changes review", for this PR.

Additional information

Viewing API changes

For convenient view of the API changes made by this PR, refer to the URLs provided in the table in the Generated ApiView comment added to this PR. You can use ApiView to show API versions diff.

Suppressing failures

If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the suppressions guide to get approval.

Getting help

  • First, please carefully read through this PR description, from top to bottom. Please fill out the Purpose of this PR and Due diligence checklist.
  • To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
  • For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure and https://aka.ms/ci-fix.
  • For help with PR workflow diagram Step 2 (ARM review), see https://aka.ms/azsdk/pr-arm-review.
  • If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run. This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
  • If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

allenjzhang avatar Jan 23 '24 21:01 allenjzhang

Next Steps to Merge

Next steps that must be taken to merge this PR:
  • ❌ Your PR is in purview of ARM review (label: ARMReview). Please ensure your PR is on the ARM PR review queue - see instructions in steps 1 and 2 in the PR description diagram.
  • ❌ The required check named Automated merging requirements met has failed. This is the final check that must pass. Refer to the check in the PR's 'Checks' tab for details on how to fix it. In addition, refer to step 4 in the PR workflow diagram (even if your PR is for data plane, not ARM).

Swagger Validation Report

️️✔️BreakingChange succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️Breaking Change(Cross-Version) succeeded [Detail] [Expand]
There are no breaking changes.
️️✔️CredScan succeeded [Detail] [Expand]
There is no credential detected.
️⚠️LintDiff: 1 Warnings warning [Detail]
compared tags (via openapi-validator v2.1.6) new version base version
package-2021-10-01-preview package-2021-10-01-preview(2453769) default(main)

[must fix]The following errors/warnings are introduced by current PR:

Rule Message Related RPC [For API reviewers]
:warning: PreviewVersionOverOneYear The API version:2021-10-01-preview having been in a preview state over one year , please move it to GA or retire.
Location: Microsoft.Contoso/preview/2021-10-01-preview/openapi.json#L5
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️❌TypeSpecAPIView: 0 Errors, 1 Warnings failed [Detail]
Rule Message
:warning: Failed to generate TypeSpec APIView. Please check the detail log and make sure TypeSpec compiler version is the latest. "How to fix":"Check the detailed log and verify if the TypeSpec emitter is able to create API review file for the changes in PR."
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️❌PR Summary: 0 Errors, 0 Warnings failed [Detail]
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Posted by Swagger Pipeline | How to fix these errors?

Swagger Generation Artifacts

️️✔️ApiDocPreview succeeded [Detail] [Expand]
 Please click here to preview with your @microsoft account. 
️️✔️SDK Breaking Change Tracking succeeded [Detail] [Expand]

Breaking Changes Tracking

Posted by Swagger Pipeline | How to fix these errors?

Generated ApiView

Language Package Name ApiView Link
Swagger Microsoft.Contoso https://apiview.dev/Assemblies/Review/7ced84594b0a41aab472ddec0faf7689?revisionId=d63efb908fc6407580639b442d9e5458

Hi @allenjzhang! For review efficiency consideration, when creating a new API version, it is required to place API specs of the base version in the first commit, and push new version updates into successive commits. You can use OpenAPIHub to initialize the PR for adding a new version.
For more details refer to the wiki.

Hi, @allenjzhang, our workflow has detected that there is no management SDK ever released for your RP, to further process SDK onboard for your RP, you should have the SDK client library name of your RP reviewed and approved. Action Required:

  • Follow this guidance Naming for new initial management or client libraries (new SDKs) - Overview (azure.com) to create an issue for management client library name arch board review.
  • Paste the issue link that you created in step 1 in this PR
  • Impact: SDK release owner will take the approved management client library name to release SDK. No client library name approval will leads to SDK release delayed.

    Hi @allenjzhang This PR was flagged for introducing a new RP namespace that is not being onboarded with RPaaS. Merging this PR to the main branch will be blocked as RPaaS is required for new RPs. To resolve this and allow the PR to be merged into the main branch, please use RPaaS to onboard the new RP.

    PR validation pipeline can not start as the pull request is not merged or mergeable - most likely it has merge conflicts.

    @weshaggard, @mikeharder the Staging Swagger LintDiff error on PatchPropertiesCorrespondToPutProperties is not correct. Looks like it did not follow the ref link to find complete properties list. I know it is staging, still it would occur for all ARM specs. Who should I file bug to get this fixed?

    allenjzhang avatar Jan 26 '24 22:01 allenjzhang

    @weshaggard, @mikeharder the Staging Swagger LintDiff error on PatchPropertiesCorrespondToPutProperties is not correct. Looks like it did not follow the ref link to find complete properties list. I know it is staging, still it would occur for all ARM specs. Who should I file bug to get this fixed?

    @AkhilaIlla @rkmanda FYI

    konrad-jamrozik avatar Jan 26 '24 23:01 konrad-jamrozik

    @weshaggard, this is ready to merge. At least the out-of-box project has not obvious LintDiff/Model validation errors.

    allenjzhang avatar Jan 28 '24 23:01 allenjzhang

    @konrad-jamrozik , what is the PR Summary failure about? I did checked appropriate boxes in the description.

    allenjzhang avatar Jan 29 '24 00:01 allenjzhang

    @allenjzhang lets please get this green so we can be sure things are working. For the ARM review requirement we should just apply the approval label to get that green.

    weshaggard avatar Jan 29 '24 19:01 weshaggard

    @konrad-jamrozik , what is the PR Summary failure about? I did checked appropriate boxes in the description.

    @allenjzhang you tripped over this:

    • https://github.com/Azure/azure-sdk-tools/issues/6673

    I.e. automation wants for you to do this:

    • https://github.com/Azure/azure-rest-api-specs/pull/27502#issuecomment-1906925946

    I am attempting to circumvent it by adding label RPaaS. Let's see if it works.

    But I am unsure if this is what we really want. The automation would add RPaaS itself if it would find this int he readme.md:

    function isRPSaaS(readmeFilePath: string) {
      const readmeParser = new ReadmeParser(readmeFilePath);
      const openapiSubtype = readmeParser.getGlobalConfigByName("openapi-subtype");
      console.log(
        `readmeFilePath: ${readmeFilePath} openapi-subtype: ${openapiSubtype}`
      );
      return openapiSubtype === "rpaas" || openapiSubtype === "providerHub";
    }
    

    Can you verify this is what we want from the sample?

    konrad-jamrozik avatar Jan 29 '24 19:01 konrad-jamrozik

    /azp run

    konrad-jamrozik avatar Jan 29 '24 19:01 konrad-jamrozik

    Azure Pipelines successfully started running 4 pipeline(s).

    azure-pipelines[bot] avatar Jan 29 '24 19:01 azure-pipelines[bot]

    @allenjzhang to follow-up to my previous comment here, adding the RPaaS label doesn't unblock the automation. But you should be able to unblock it by adding openapi-subtype: rpaas or openapi-subtype: providerHub to readme.md as e.g. seen here, on line 28:

    image

    konrad-jamrozik avatar Jan 29 '24 19:01 konrad-jamrozik

    @konrad-jamrozik It doesn't show me that RPSaaS check blocks this PR, is that fine?

    visingla-ms avatar Jan 30 '24 23:01 visingla-ms

    @visingla-ms I am already working with @allenjzhang to address this. See here: https://github.com/Azure/azure-rest-api-specs/pull/27502#issuecomment-1915427553

    konrad-jamrozik avatar Jan 31 '24 00:01 konrad-jamrozik

    @allenjzhang, @konrad-jamrozik - Does this PR actually need review by ARM? I see @visingla-ms left a couple of unaddressed comments. If it doesn't require review, please keep it out of the queue (leave it in ARMChangesRequested state).

    mentat9 avatar Feb 06 '24 20:02 mentat9

    Please address or respond to feedback from the ARM API reviewer.
    When you are ready to continue the ARM API review, please remove the ARMChangesRequested label.
    This will notify the reviewer to have another look.
    If the feedback provided needs further discussion, please use this Teams channel to post your questions - aka.ms/azsdk/support/specreview-channel.
    Please include [ARM Query] in the title of your question to indicate that it is ARM-related.

    No, I don't think it needs to. Was trying to follow the process outlined. Will do.

    allenjzhang avatar Feb 06 '24 20:02 allenjzhang

    @mentat9 sorry for the confusion. We will bypass the ARM review. @allenjzhang to bypass it, we need to ensure that from labels with ARM in them, only ARMReview and ARMSignedOff labels are present.

    konrad-jamrozik avatar Feb 07 '24 18:02 konrad-jamrozik

    Hi, @allenjzhang. Your PR has no update for 14 days and it is marked as stale PR. If no further update for over 14 days, the bot will close the PR. If you want to refresh the PR, please remove no-recent-activity label.

    Hi, @allenjzhang. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @allenjzhang. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.

    Hi, @allenjzhang. The PR will be closed since the PR has no update for 28 days. If you still need the PR review to proceed, please reopen it and @ mention PR assignee.