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.

Sign up for GitHub

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

Jump to bottom

Add sdk_customization.md #10861

Merged

PhoenixHe-NV merged 6 commits into Azure:master from PhoenixHe-NV:tih/sdk-automation-doc

Sep 30, 2020

Contributor

PhoenixHe-NV commented Sep 21, 2020 •

edited by m-nash

Loading

MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.

Contribution checklist:

I commit to follow the Breaking Change Policy of “no breaking changes
I have reviewed the documentation for the workflow.
Validation tools were run on swagger spec(s) and errors have all been fixed in this PR. How to fix?

If any further question about AME onboarding or validation tools, please view the FAQ.

ARM API Review Checklist

Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
- Adding new API(s)
- Adding a new API version
- Adding a new service
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.

Breaking Change Review Checklist

If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.

Removing API(s) in stable version
Removing properties in stable version
Removing API version(s) in stable version
Updating API in stable version with Breaking Change Validation errors
Updating API(s) in preview over 1 year

Please follow the link to find more details on PR review process.


          Add sdk_customization.md

eb27314

PhoenixHe-NV added the DoNotMerge label

PhoenixHe-NV requested a review from weshaggard

September 21, 2020 09:30

openapi-assignment-bot bot assigned akning-ms

openapi-pipeline-app bot commented Sep 21, 2020 •

edited

Loading

[Staging] Swagger Validation Report

️️✔️BreakingChange [Detail]

 There are no breaking changes.

️️✔️LintDiff [Detail]

 Validation passes for LintDiff.

️️✔️Avocado [Detail]

 Validation passes for Avocado.

️️✔️ModelValidation [Detail]

 Validation passes for ModelValidation.

️️✔️SemanticValidation [Detail]

 Validation passes for SemanticValidation.

_{Posted by Swagger Pipeline | How to fix these errors?}

azure-pipelines bot commented Sep 21, 2020

Azure Pipelines successfully started running 1 pipeline(s).

openapi-sdkautomation bot commented Sep 21, 2020 •

edited

Loading

azure-sdk-for-python - 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 bot commented Sep 21, 2020 •

edited

Loading

azure-sdk-for-go - 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 bot commented Sep 21, 2020 •

edited

Loading

Azure CLI Extension Generation - 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 bot commented Sep 21, 2020 •

edited

Loading

azure-sdk-for-js - 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 bot commented Sep 21, 2020 •

edited

Loading

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 bot commented Sep 21, 2020 •

edited

Loading

azure-sdk-for-net - 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 bot commented Sep 21, 2020 •

edited

Loading

azure-sdk-for-python-track2 - 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 bot commented Sep 21, 2020 •

edited

Loading

azure-resource-manager-schemas - 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 bot commented Sep 21, 2020 •

edited

Loading

Trenton Generation - 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


          Update doc with detail steps

4a862e7

azure-pipelines bot commented Sep 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).


          Add packageName in generateOutput

8e53ca9

azure-pipelines bot commented Sep 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).


          Use propertyNames

0326b55

ArcturusZhang reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md Outdated Show resolved Hide resolved

azure-pipelines bot commented Sep 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).


          Fix logPrefix

6e64d41

azure-pipelines bot commented Sep 22, 2020

Azure Pipelines successfully started running 1 pipeline(s).

weshaggard reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md Outdated

+              ### SwaggerToSdkConfig
+              This is type of file `./swagger_to_sdk_config.json` in sdk repo.
+              The running environment of these scripts would be expected to be __Ubuntu 18.04__ on Azure Pipeline. All the running script should be executable.

Member

weshaggard Sep 24, 2020

I think it is fine to call out that linux is needed but we should be careful about being specific to Ubuntu 18.04. While that might be what we use today I expect that will update to newer version later.

In the future I think we will likely want to add some sort of advanced option that would allow languages to specify a docker container they can use to run the steps in instead of using the init script, but hopefully we don't need that initially.

Contributor Author

PhoenixHe-NV Sep 25, 2020

Makes sense. Currently let's make it simple.

Contributor Author

PhoenixHe-NV Sep 25, 2020

If you really want to use docker image, you can pull it in init and run everything in the image with folder mount I think.

weshaggard reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md

+                      "generateScript"
+                    ]
+                  },
+                  "packageOptions": {

Member

weshaggard Sep 24, 2020

As we discussed from an implementation priority prospective I'd like to get everything up through the genreateScript options. If we can make conversion from existing config files easier without adding all the extra complexity these extra steps we should try to avoid those. The simpler we can make the automation the better. If we can solve some of the complexity with some shared code that would be better then having these extra round tripping and potential error modes between the language repo and the automation.

Contributor Author

PhoenixHe-NV Sep 25, 2020

Agree. Will implement generateOption first.

weshaggard reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md

+                      },
+                      {
+                        // If set to true, any line of log will hit
+                        "type": "boolean"

Member

weshaggard Sep 24, 2020

We could consider using only a regex and use .* for places where it takes everything.

Contributor Author

PhoenixHe-NV Sep 25, 2020

But what if you want it to match nothing?

Member

weshaggard Sep 25, 2020

You could put empty or some string that will not match anything.

weshaggard reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md

+                "headSha": "fce3400431eff281bddd04bed9727e63765b8da0",
+                "headRef": "refs/pull/1234/merge",
+                "repoGitUrl": "[email protected]:Azure/azure-rest-api-specs.git",
+                "repoHttpsUrl": "https://github.com/Azure/azure-rest-api-specs.git",

Member

weshaggard Sep 24, 2020

I don't think it necessary to provide these duplicated values it is just another place something could be out of sync, so if there is no scenario for them both I would just keep the repoUrl as the https url.

Contributor Author

PhoenixHe-NV Sep 25, 2020

OK. Remove repoGitUrl

weshaggard reviewed

View reviewed changes

documentation/sdkautomation/sdk_customization.md

+                  "sdk/cdn/cdn.snuget"
+                ],
+                "isPublic": true,
+                "downloadUrlPrefix": "https://portal.azure-devex-tools.com/api/sdk-dl-pub?p=Azure/azure-rest-api-specs/1234/azure-sdk-for-net/",

Member

weshaggard Sep 24, 2020

I'd like to see if we can use template model here for the install instructions instead of needing to provide exact links to the downloads. Perhaps there are reasons but I'd like to understand that better as we start to test these out.

Contributor Author

PhoenixHe-NV Sep 25, 2020

What do you mean by template mode? I'm afraid there would be some advanced logic such as parsing content of artifact, so I leave it as a script.

Member

weshaggard Sep 25, 2020

I will have to see how this works in practice but by template model I mean we provide a string that has template variables for the package name that the automation can fill-in with the full links on the back-end.

Contributor Author

PhoenixHe-NV Sep 26, 2020

In that case we need to provide a template execution engine which is a little bit complicated (artifacts are array) so it would be better to render it on sdk side or maybe you can share some code in eng/common.

weshaggard approved these changes

View reviewed changes

Member

weshaggard left a comment

I left some feedback based on our design review. At a high level the design seems reasonable and we should move forward with implementing it and we will make adjustments as needed as we test it.


          Update: add dryRun to collect info, fix typo

5c2b07b

azure-pipelines bot commented Sep 28, 2020

Azure Pipelines successfully started running 1 pipeline(s).

Contributor Author

PhoenixHe-NV commented Sep 28, 2020

@weshaggard I've made some update to the doc please have a look. The biggest change I want to mention here is dryRun.

Member

weshaggard commented Sep 28, 2020

I am curious what scenarios you plan to use "dryRun" for? Should we instead call it something like "parseOnly" or "doNotGenerate" something that is a little more clear on the expectation.

Contributor Author

PhoenixHe-NV commented Sep 29, 2020

I am curious what scenarios you plan to use "dryRun" for? Should we instead call it something like "parseOnly" or "doNotGenerate" something that is a little more clear on the expectation.

The idea is: some autorest generation is based on current code. For example, azure cli extension generation is planned to generate operation that are not define in code. SDK Automation's future planning also includes customized sdk code during spec PR, and to make it work correctly we need to checkout the user's branch for autorest generation (there's a dependency).

For the naming part I'm OK with "parseOnly", but how do you think after my comment?

Member

weshaggard commented Sep 29, 2020

I'm not sure I fully understand the scenario but I don't have to yet. I do think we should name the option after what the expectations are though and so people that implement the generation script has some idea of what to do. Also I'm curious what would cause the SDK Automation to set this option in the input file. Is it based on the option in the swagger_to_sdk config?

Contributor Author

PhoenixHe-NV commented Sep 30, 2020

@weshaggard Yes the input will be based on swagger_to_sdk config. So maybe we could discuss about the option and the scenario next time, and I'll merge this PR first. That "parseOnly" option will not be implemented next month before discussion.

PhoenixHe-NV merged commit 301b21c into Azure:master

iscai-msft added a commit to iscai-msft/azure-rest-api-specs that referenced this pull request


          Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

edef031

…into fix_kv_python_readme

* 'master' of https://github.com/Azure/azure-rest-api-specs: (364 commits)
  Update pull_request_assignment.yml (Azure#11026)
  [Ready For Review] a new version with changes on compute instance/vm sizes (Azure#10542)
  correct the description (Azure#11009)
  [Data plane][Azure Cognitive Search] Add encryptionKey property to indexer, datasource and skillset metadata (Azure#10839)
  Adding reseller Id (Azure#11013)
  Update Phone Number Administration Swagger to remove unnecessary parameters, and adjust API responses. (Azure#11010)
  update all swaggers to 2020-05-01 API version (Azure#10960)
  Changed the Instance Field Description to Support Primitive Data type (Azure#10995)
  Fix the acronym casing for ACSSMS (Azure#10988)
  Update pull_request_assignment.yml (Azure#10974)
  Add sdk_customization.md (Azure#10861)
  [Hub Generated] Review request for Microsoft.DataMigration to add version preview/2018-07-15-preview (Azure#10494)
  Make version int64 (Azure#10984)
  Microsoft.Security/iotDefenderSettings (2020-08-06-preview) (Azure#10810)
  added incident alerts and incident bookmarks APIs (Azure#10806)
  add publicNetworkAccess property to datafactory (Azure#10956)
  Fix CDN matchValues and selector fields to be optional (Azure#10950)
  [Microsoft.StorageSync] Introduce 2020-09-01 API version (Azure#10836)
  strings update (Azure#10969)
  Update QnAMaker.json - response code correction (Azure#10828)
  ...

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

Labels