Skip to content

Comments

[Hub Generated] Review request for Microsoft.ApiManagement to add version preview/2019-12-01-preview#8066

Merged
mmyyrroonn merged 15 commits intoAzure:masterfrom
solankisamir:solanki/apim-2019-12-01-preview
Jan 16, 2020
Merged

[Hub Generated] Review request for Microsoft.ApiManagement to add version preview/2019-12-01-preview#8066
mmyyrroonn merged 15 commits intoAzure:masterfrom
solankisamir:solanki/apim-2019-12-01-preview

Conversation

@solankisamir
Copy link
Member

@solankisamir solankisamir commented Jan 3, 2020

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@solankisamir
Copy link
Member Author

This is PR related to #8046. We couldn't the apim-2019-12-01-preview branch as it was read-only. Hence created a clone here and working on edits here.

@openapi-sdkautomation
Copy link

openapi-sdkautomation bot commented Jan 3, 2020

azure-sdk-for-net - Release

failed [Logs] [Expand Details]
  • Generate from 13fb704 with merge commit ec0ae41. SDK Automation 13.0.17.20191226.1
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
    [AutoRest] realpath(): Permission denied
  • Microsoft.Azure.Management.ApiManagement [Logs]  [Release SDK Changes]
      Failed to create the package Microsoft.Azure.Management.ApiManagement.
      Error: dotnet msbuild build.proj /t:CreateNugetPackage /p:Scope=apimanagement /v:n /p:SkipTests=true , {} 

    @openapi-sdkautomation
    Copy link

    openapi-sdkautomation bot commented Jan 3, 2020

    azure-sdk-for-go - Release

    ️✔️ succeeded [Logs] [Expand Details]

    @openapi-sdkautomation
    Copy link

    openapi-sdkautomation bot commented Jan 3, 2020

    azure-sdk-for-js - Release

    ️✔️ succeeded [Logs] [Expand Details]
    • ️✔️ Generate from 13fb704 with merge commit ec0ae41. SDK Automation 13.0.17.20191226.1
    • ️✔️@azure/arm-apimanagement [Logs]  [Release SDK Changes]
      [npmPack] npm WARN deprecated rollup-plugin-node-resolve@5.2.0: This package has been deprecated and is no longer maintained. Please use @rollup/plugin-node-resolve.
      [npmPack] loaded rollup.config.js with warnings
      [npmPack] (!) Unused external imports
      [npmPack] default imported from external module 'rollup' but never used
      [npmPack] 
      [npmPack] ./esm/apiManagementClient.js → ./dist/arm-apimanagement.js...
      [npmPack] created ./dist/arm-apimanagement.js in 1.7s

    @openapi-sdkautomation
    Copy link

    openapi-sdkautomation bot commented Jan 3, 2020

    azure-sdk-for-java - Release

    No readme.md specification configuration files were found that are associated with the files modified in this pull request, or swagger_to_sdk section in readme.md is not configured

    @openapi-sdkautomation
    Copy link

    openapi-sdkautomation bot commented Jan 3, 2020

    azure-sdk-for-python - Release

    ️✔️ succeeded [Logs] [Expand Details]
    • ️✔️ Generate from 13fb704 with merge commit ec0ae41. SDK Automation 13.0.17.20191226.1
    • ️✔️azure-mgmt-apimanagement [Logs]  [Release SDK Changes]
      [build_package] /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'long_description_content_type'
      [build_package]   warnings.warn(msg)
      [build_package] warning: no files found matching '*.py' under directory 'tests'
      [build_package] warning: no files found matching '*.yaml' under directory 'tests'
      [build_package] /usr/lib/python3.6/distutils/dist.py:261: UserWarning: Unknown distribution option: 'long_description_content_type'
      [build_package]   warnings.warn(msg)
      [build_package] warning: no files found matching '*.py' under directory 'tests'
      [build_package] warning: no files found matching '*.yaml' under directory 'tests'

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @solankisamir
    Copy link
    Member Author

    @NullMDR I am trying to fix the prettier issues using https://github.com/Azure/azure-rest-api-specs/blob/master/documentation/ci-fix.md, but I am getting this error

    D:\github\azure-rest-api-specs>npm install prettier-fix
    npm ERR! code E404
    npm ERR! 404 Not Found - GET https://registry.npmjs.org/prettier-fix - Not found
    npm ERR! 404
    npm ERR! 404 'prettier-fix@latest' is not in the npm registry.
    npm ERR! 404 You should bug the author to publish it (or use the name yourself!)
    npm ERR! 404
    npm ERR! 404 Note that you can also install from a
    npm ERR! 404 tarball, folder, http url, or git url.

    npm ERR! A complete log of this run can be found in:
    npm ERR! C:\Users\sasolank\AppData\Roaming\npm-cache_logs\2020-01-05T19_20_03_381Z-debug.log

    @solankisamir solankisamir added WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required API Management labels Jan 5, 2020
    @PhoenixHe-NV
    Copy link

    @solankisamir You need

    npm install; npm run prettier -- --write "specification/apimanagement/**/*.json"

    @solankisamir
    Copy link
    Member Author

    solankisamir commented Jan 6, 2020

    @NullMDR ran the command and hit the issue below. I don't have the azure-rest-api-specs-test repos locally. Do I need to get that?
    debug_log

    PS D:\github\azure-rest-api-specs> npm install; npm run prettier -- --write "specification/apimanagement/**/*.json"

    azure-rest-api-specs-tests@0.1.0 postinstall D:\github\azure-rest-api-specs
    scripts/switch-to-preproduction.sh
    npm ERR! errno 1
    npm ERR! azure-rest-api-specs-tests@0.1.0 postinstall: scripts/switch-to-preproduction.sh
    npm ERR! Exit status 1
    npm ERR!
    npm ERR! Failed at the azure-rest-api-specs-tests@0.1.0 postinstall script.
    npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

    npm ERR! A complete log of this run can be found in:
    npm ERR! C:\Users\sasolank\AppData\Roaming\npm-cache_logs\2020-01-06T16_40_16_473Z-debug.log
    npm ERR! missing script: prettier

    npm ERR! A complete log of this run can be found in:
    npm ERR! C:\Users\sasolank\AppData\Roaming\npm-cache_logs\2020-01-06T16_40_18_669Z-debug.log

    Copy link
    Contributor

    @ryansbenson ryansbenson left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    One minor change

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    You shouldn't have an id field in the properties section of a resource. It conflicts with the top level id property. Please find a different name.

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    It's on the top level, not inside "properties".

    Copy link
    Member Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @ryansbenson please see @vfedonkin comment. This is a contract associated with a proxy resource and we are just adding id to fix the ARM warnings generated as part of live traffic.

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @ryansbenson, @solankisamir , I think we will need to modify all entities to have our fields inside "properties", but it should be done in our next version, since we already behind the schedule for the version we trying to release.

    Copy link
    Member Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @ryansbenson a gentle ping on this!

    Copy link
    Member Author

    @solankisamir solankisamir Jan 15, 2020

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @ryansbenson we fixed this in the spec to unblock it. Can you review again?

    @PhoenixHe-NV
    Copy link

    @solankisamir Please merge from latest master to pick up latest scripts and try again

    solankisamir and others added 6 commits January 7, 2020 15:53
    * ApiManagement - renaming /properties entity to /namedValues
    
    * ApiManagement - renaming parameter name: propId to namedValueId
    * ApiManagement - async operatins for NamedValues and ApiSchemas
    
    * ApiManagement - client secrets in IdentityProvider
    
    * ApiManagement - json format fixes
    
    * ApiManagement - examples fixes
    
    * ApiManagement - fixed identtyProviderContract
    
    * ApiManagement - secrets in subscriptions
    
    * ApiManagement - secrets in OpenIdConnectProviders and AuthServers
    
    * ApiManagement - secrets in TenantAccess
    
    * ApiManagement - fix example
    
    * ApiManagement - fixes
    * ApiManagement - tagDescription id
    
    * ApiManagement - policyDescriptions
    
    * ApiManagement - policyDescriptions small fix
    
    * ApiManagement - policyDescriptions sample
    
    * ApiManagement - revisions id and api schema doc
    
    * ApiManagement - tag descriptions list example
    
    * ApiManagement - ciphers info
    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn mmyyrroonn closed this Jan 16, 2020
    @mmyyrroonn mmyyrroonn reopened this Jan 16, 2020
    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn
    Copy link
    Contributor

    /azp run automation - sdk

    @azure-pipelines
    Copy link

    Azure Pipelines successfully started running 1 pipeline(s).

    @mmyyrroonn mmyyrroonn merged commit ec0ae41 into Azure:master Jan 16, 2020
    @mmyyrroonn
    Copy link
    Contributor

    @ryansbenson sorry! I made a mistake to merge it. Could you please review this PR, if still need some change we can open another or revert this one. @solankisamir Sorry. Please hold on the following process since ARM didn't sign off yet. Thanks.

    @PhoenixHe-NV
    Copy link

    @myronfanqiu You can create a PR to revert this one

    @sanjaiganesh
    Copy link
    Contributor

          }
    

    For all async operations, add x-ms-long-running-operation:true and

    , add "x-ms-long-running-operation-options" extension with "final-state-via" property.

    "x-ms-long-running-operation-options": {
      "final-state-via": "location"
    }

    or

    "x-ms-long-running-operation-options": {
      "final-state-via": "azure-async-operation"
    }

    Refers to: specification/apimanagement/resource-manager/Microsoft.ApiManagement/preview/2019-12-01-preview/apimapis.json:2361 in 13fb704. [](commit_id = 13fb704, deletion_comment = False)

    "responses": {
    "202": {
    "description": "Request to update NamedValue was accepted.",
    "headers": {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    For all async operations, add x-ms-long-running-operation:true and

    , add "x-ms-long-running-operation-options" extension with "final-state-via" property.

    "x-ms-long-running-operation-options": {
      "final-state-via": "location"
    }

    or

    "x-ms-long-running-operation-options": {
      "final-state-via": "azure-async-operation"
    }

    "paths": {
    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ApiManagement/service/{serviceName}/namedValues": {
    "get": {
    "tags": [
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    missing ListBySubscrition and ListByResourceGroup calls

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We don't have such methods, "ListByService" only

    }
    ],
    "responses": {
    "202": {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    add sync response 200 as well.. as service could respond synchronously if no changes.

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    We use 204 for "update was successful" since no any data returned

    "Location": {
    "description": "The URL where the status of the long running operation can be checked.",
    "type": "string"
    }
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    check other comments on LRO tracking extension annotation

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    @sanjaiganesh , should we remove this header description and use this instead?:

    "x-ms-long-running-operation-options": {
    "final-state-via": "location"
    }

    @sanjaiganesh sanjaiganesh added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Jan 17, 2020
    @sanjaiganesh
    Copy link
    Contributor

    looks fine.. added few comments. please address them in separate PR. added armsignoff.

    @solankisamir
    Copy link
    Member Author

    looks fine.. added few comments. please address them in separate PR. added armsignoff.

    @sanjaiganesh Thanks for the review. @vfedonkin can you create another PR with the changes requested.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    API Management ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    7 participants