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

Data plane codegen docs #22552

Closed
wants to merge 4 commits into from

Conversation

mikekistler
Copy link
Member

The PR move the docs for dataplane codegen from the wiki to the repo and makes a first round of updates / additions.

More work is needed -- some specific areas are identified in the document as notes to reviewers.

@mikekistler mikekistler marked this pull request as draft January 19, 2022 05:04
@mikekistler mikekistler requested a review from lmazuel January 19, 2022 05:05
@check-enforcer
Copy link

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run python - [service] - ci

@lmazuel lmazuel requested review from iscai-msft and msyyc January 19, 2022 17:00
The `module_name` is the name for the specific feature or function of the REST API that underlies this SDK.
Usually this name comes from the name of the REST API file itself or one of the directories toward the end of the file path.

In Python, a project's `package name` will normally be `azure-{service_name}-{module_name}`. After release, you can find it in pypi.
Copy link
Member

Choose a reason for hiding this comment

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

This can be more subtle than "azure-servicename-modulename", for instance "azure-data-tables". or no modulename if the service is single, like "azure-servicebus".
I would say usually, modulename is optional if the service is single, and required if the service has several packages (azure-synapse-accesscontrol).

```
````

**Reviewers: which of the above can we remove? We should keep only those needed for the "common" case. And if any could just be the default (e.g. license-header), we should remove those too. Then we should document whatever remains.**
Copy link
Member

Choose a reason for hiding this comment

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

The title should be extracted from the Swagger titlte, so title should be used only if the Swagger titlte is not the expected client titlte for some reason.

We have our ongoing discussion on credentials (that would be two flags less).

I don't think we can remove anything else at the momemt, even the licence header I think is not default to MICROSOFT_MIT_NO_VERSION....

We could make a rule to deduce the default namespace from the package name (cc @iscai-msft).

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i think that's fair, made an issue for it

Now you can run this command in swagger folder you just created.

```sh
autorest --version=latest --python --use=@autorest/python@latest README.md
Copy link
Member

Choose a reason for hiding this comment

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

You should not need --version=latest and the --use. We should also tell people to do a autorest --reset. Autorest cache is really conservative, and if you use autorest over several months, you may end up with something super old. Reset delete the cache. I reached a level of confused people, that I always tell then to do a autorest --reset before any commands.


Here is [template files](https://github.com/Azure/azure-sdk-for-python/tree/main/scripts/quickstart_tooling_llc/template), and you at least need to edit item (like `{{package_name}}`, `{{package_pprint_name}}`, etc) in `CHANGELOG.md`, `README.md`, `setup.py`, `sdk_packaging.toml` according to specific info of your own service. It is easier to understand how to edit the files with reference existing service: [webpubsub](https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/webpubsub/azure-messaging-webpubsubservice)

**Reviewers: I understand that work is in progress to generate all these files with a special "init" flag to autorest, so this item will probably go away in the near future.**
Copy link
Member

Choose a reason for hiding this comment

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


## How to add convenience code

**Reviewers: Somewhere, and here seems like as good a place as any, we need to tell teams how to add convenience / "grow up" code. There are certainly right and wrong ways to do this, so we need to give service teams some guidance on this. We could just give the basics here and then point to another document with full details, since most service teams won't bother to add convenience code, but we need the information here in case they do.**
Copy link
Member

Choose a reason for hiding this comment

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

Yes, we were discussing with @iscai-msft we need this doc once she's done with MetricsAdvisor playing to understand all possible scenario.s


## Setup your repo

- Fork and clone the azure-sdk-for-python repo.
Copy link
Contributor

Choose a reason for hiding this comment

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

Great if we could include a hyperlink to azure-sdk-for-python

Three key pieces of information for your project are the `service_name`, `module_name`, and `package_name`.

The `service_name` is the short name for the Azure service. The `service_name` should match across all the SDK language repos
and is generally name of the directory in the specification folder of the azure-rest-api-specs repo that contains the REST API definition file.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "generally the name"

Also just spitballing: should we include examples here? "For example, azure-rest-api-specs/specification/resource" corresponds to resource being the service_name"

Copy link
Member Author

Choose a reason for hiding this comment

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

We should include an example here. I thought I would just use Web Pub Sub since we also use it below. But I'm not sure it is a good example.

  • the API definitions for Web Pub Sub are stored within the specifications/webpubsub directory of azure-rest-api-specs. That suggests that the service_name should be "webpubsub".
  • but deconstructing the package name a few lines down based on the template shown indicates that the actual service_name for Web Pub Sub is "messaging".

Which may be fine to give as an example that doesn't follow the general rule. But it makes me wonder whether there are more exceptions than there are cases that follow the rule.

I think I need to soften the language here, which is disappointing but maybe necessary to reflect reality.

All the files for your SDK will reside in the **${PROJECT_ROOT} folder**, where

```sh
${PROJECT_ROOT}=sdk/{service_name}/azure-{service_name}-{module_name}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we use powershell-esque definitions? don't feel too strongly, just want to double click

Now you can run this command in swagger folder you just created.

```sh
autorest --version=latest --python --use=@autorest/python@latest README.md
Copy link
Contributor

Choose a reason for hiding this comment

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

you can even include python: true in the readme. Also you don't need --version=latest and --use=@autorest/python@latest. So the command can just end up being autorest README.md


- `setup.py`: most important files about dependency and supported version for Python.

Here is [template files](https://github.com/Azure/azure-sdk-for-python/tree/main/scripts/quickstart_tooling_llc/template), and you at least need to edit item (like `{{package_name}}`, `{{package_pprint_name}}`, etc) in `CHANGELOG.md`, `README.md`, `setup.py`, `sdk_packaging.toml` according to specific info of your own service. It is easier to understand how to edit the files with reference existing service: [webpubsub](https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/webpubsub/azure-messaging-webpubsubservice)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Here are some template files"


## How to add convenience code

**Reviewers: Somewhere, and here seems like as good a place as any, we need to tell teams how to add convenience / "grow up" code. There are certainly right and wrong ways to do this, so we need to give service teams some guidance on this. We could just give the basics here and then point to another document with full details, since most service teams won't bother to add convenience code, but we need the information here in case they do.**
Copy link
Contributor

Choose a reason for hiding this comment

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

Good point @mikekistler , I'm starting work on a "grow up doc" at the same time we're designing how to make it more convenient to edit generated code, so I'll be working on this!


Here is the [Dataplane Codegen Quick Start for Test](https://github.com/Azure/azure-sdk-for-python/wiki/Dataplane-Codegen-Quick-Start-for-Test) about how to write and run tests for the Python SDK.

**Reviewers: We may want to pull the content from this wiki page into here.**
Copy link
Contributor

Choose a reason for hiding this comment

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

As a note: @msyyc has been doing some work here, to automatically generate basic tests and sample templates. I think there will still be some time until we move that code into autorest bc we have other more pressing things first, but just wanted to give you a heads up


If there's already a `ci.yml` file in your project path. then the only thing you need to do is to add the `Artifacts name` and `safeName` of yours into that `ci.yml`.

Usually, `Artifacts name` is same as `package name` and remove `-` in `Artifacts name` to get `safeName`. Example: [webpubwub](https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/webpubsub/ci.yml), [purview](https://github.com/Azure/azure-sdk-for-python/blob/main/sdk/purview/ci.yml)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to run /prepare-pipelines or something in your PR if there has been no ci.yml / pipelines set up. The PR recommends it too


You should add samples in the `samples` directory under **${PROJECT_ROOT}**. Each sample should have its own folder and contain a `Readme.md`([example](https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/webpubsub/azure-messaging-webpubsubservice/samples/Readme.md)) which introduces simply how to run the sample.

If you want to run sample with dev mode, just run `pip install -e .`([detail doc](https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-e)) under **${PROJECT_ROOT}** instead of `pip install azure.myservice`.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you want to run sample with dev mode, just run `pip install -e .`([detail doc](https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-e)) under **${PROJECT_ROOT}** instead of `pip install azure.myservice`.
If you want to run sample with dev mode, just run `pip install -e .`([detail doc](https://pip.pypa.io/en/stable/cli/pip_install/#cmdoption-e)) under **${PROJECT_ROOT}** instead of `pip install azure-myservice`.

mikekistler and others added 2 commits January 19, 2022 20:19
Co-authored-by: Laurent Mazuel <lmazuel@microsoft.com>
Co-authored-by: Laurent Mazuel <lmazuel@microsoft.com>
@mikekistler
Copy link
Member Author

I let this PR get stale so I've just taken the changes and incorporated then back into the wiki. I'd be happy to help with another effort to move the instructions into the repo if desired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants