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

add --package-mode for new service to package #1112

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion autorest/codegen/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,21 @@ def _validate_code_model_options(options: Dict[str, Any]) -> None:
"If you want operation files, pass in flag --show-operations"
)

if options["package_mode"] == 'dataplane' and \
Copy link
Member

Choose a reason for hiding this comment

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

The value "dataplane" should not be hardcoded, we should have a map of value to folder, and the template folder should be part of the autorest delivery

For instance, the folder autorest/codegen/packaging_template/dataplane contains the dataplane template, and the folder autorest/codegen/packaging_template/abcxyz will contain the template abcxyz

Copy link
Member Author

@msyyc msyyc Jan 29, 2022

Choose a reason for hiding this comment

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

template could be distinguished with different folder:
image we could make a map with that.

And different package-mode may need different parameters, so here we had better keep it.

not all([options["package_name"], options["client_name"], options["credential_scopes"],
options["package_pprint_name"], options["output_folder"], options["input_file"]]):
raise ValueError(
"--package-name, --title, --package-pprint-name, --credential-scopes, --output-folder"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think all of these flags need to be required for package-mode to work. For example, at least for title, we can get the value from the client name

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, most of those flags will be required anyway, and we should get the values from the codemodel, once they are solved and resolved. For instance, the codemodel will always have a title.

For options that are not necessary today to generate, see that I did a proposal in the initial issue, and it was not to add new flags.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @lmazuel @iscai-msft, when people want to generate a shippable package, these flags(package_name, output_folder, etc) are necessary in swagger/readme.md. Rather than that let users configure readme.md then generate package with the configuration, I think it is more convenient to do that with one step: force users to input necessary flags with 'package-mode`. After generation, users could get shippable package directly. It may be a little different with #1053, but it shall be convenient for users, too.

title: although it is usually defined in swagger, but most of it is invalid for SDK like this. So I think it is better to let users input for confirmation.

I try to let users input less parameters as soon as possible, now they only need 6.

Copy link
Contributor

@iscai-msft iscai-msft Feb 1, 2022

Choose a reason for hiding this comment

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

@msyyc I'm kind of confused, wouldn't people be running --package-mode with generation, so there's no need to add duplicate flags explicitly for pakcage mode?

For example, --title shouldn't be required because that's a flag to override the client name in the swagger. Don't we want the package name's client to be the same as the generated client's name? So i don't get why you want to require --title for package mode. Honestly the only flag I think we may need to do a check on is --package-pprint-name, and I'm not sure we even have to require this flag, we could default to the client name if the flag is not specified

" --input-file are necessary"
)

if options["package_mode"] == 'dataplane' and options["azure_arm"]:
raise ValueError(
"--package_mode=dataplane and --azure-arm can not be used at the same time"
)



_LOGGER = logging.getLogger(__name__)
class CodeGenerator(Plugin):
@staticmethod
Expand Down Expand Up @@ -134,7 +149,7 @@ def _create_code_model(self, yaml_data: Dict[str, Any], options: Dict[str, Union
code_model.setup_client_input_parameters(yaml_data)

# Get my namespace
namespace = self._autorestapi.get_value("namespace")
namespace = code_model.options.get("namespace", self._autorestapi.get_value("namespace"))
Copy link
Member

Choose a reason for hiding this comment

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

@iscai-msft why would there be a namesapce in the code_model.options?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah you're right there isn't any, @msyyc I don't think we need to change the code here

Copy link
Member Author

@msyyc msyyc Jan 29, 2022

Choose a reason for hiding this comment

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

namesapce in the code_model.options is needed to render template for package-mode. Please see here: https://github.com/Azure/autorest.python/pull/1112/files#r794987407
image

To make it more clear, I could change it like here:
image

_LOGGER.debug("Namespace parameter was %s", namespace)
if not namespace:
namespace = yaml_data["info"]["python_title"]
Expand Down Expand Up @@ -289,6 +304,12 @@ def _build_code_model_options(self) -> Dict[str, Any]:
"low_level_client": low_level_client,
"combine_operation_files": self._autorestapi.get_boolean_value("combine-operation-files", version_tolerant),
"python3_only": python3_only,
"package_mode": self._autorestapi.get_value("package-mode"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Some of these are already in the options dict / can be get other ways. For example. client_name, credential_scopes, output_folder, and input_file

Copy link
Member Author

@msyyc msyyc Jan 26, 2022

Choose a reason for hiding this comment

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

Those parameters is to render template for package-mode. I don't see where I could get them except in options.

Copy link
Contributor

Choose a reason for hiding this comment

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

@msyyc, we already have informnation for all of the flags you've added here, besides package-pprint-name, this is doubling code for no real reason. If you go through the code here you can see how we're already storing this information on the code model (not necessarily just in options)

"client_name": self._autorestapi.get_value("title"),
"credential_scopes": self._autorestapi.get_value("credential-scopes"),
"package_pprint_name": self._autorestapi.get_value("package-pprint-name"),
"output_folder": self._autorestapi.get_value("output-folder"),
"input_file": self._autorestapi.get_value("input-file"),
}

if options["builders_visibility"] is None:
Expand All @@ -306,8 +327,16 @@ def _build_code_model_options(self) -> Dict[str, Any]:
if azure_arm:
options["credential"] = True
options["head_as_boolean"] = True

if options["package_mode"] == 'dataplane':
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think adding these options is in the scope of this PR, not sure all of these should default to these values as well

Copy link
Member

Choose a reason for hiding this comment

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

Correct, the scope of the PR is templating, not changing behavior

Copy link
Member Author

Choose a reason for hiding this comment

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

The changing behavior is necessary to generate a whole package: package-mode only asks users to input limited parameter. So it is necessary to help configure some other parameters.
For example, codegen asks to input --credential if users want to use --credential-scopes. For convenience, we want users to input limited parameters. So package-mode only asks to input --credential-scopes. Then we have to help to replenish options["credential"] = True

options["namespace"] = options["package_name"].replace("-", '.')
options["credential"] = True
options["basic_setup_py"] = True
options["package_version"] = options["package_version"] if options["package_version"] else "1.0.0b1"

return options


def process(self) -> bool:
# List the input file, should be only one
inputs = self._autorestapi.list_inputs()
Expand Down
48 changes: 47 additions & 1 deletion autorest/codegen/serializers/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@
# Licensed under the MIT License. See License.txt in the project root for
# license information.
# --------------------------------------------------------------------------
from typing import List, Optional
from typing import List, Optional, Any
from pathlib import Path
import time
from jinja2 import PackageLoader, Environment
from autorest.codegen.models.operation_group import OperationGroup

Expand All @@ -29,6 +30,31 @@
"JinjaSerializer",
]

def _get_environment(template_path: str) -> Environment:
return Environment(loader=PackageLoader("autorest.codegen", template_path))


def _build_package_render_parameters(code_model: CodeModel) -> Any:
parameters = {}
parameters["package_name"] = code_model.options["package_name"]
parameters["package_version"] = code_model.options["package_version"]
parameters["namespace"] = code_model.options["namespace"]
folder_list = parameters["package_name"].split('-')
parameters["folder_first"] = folder_list[0]
msyyc marked this conversation as resolved.
Show resolved Hide resolved
parameters["folder_second"] = folder_list[1]
parameters["test_prefix"] = folder_list[-1]
parameters["date_to_release"] = time.strftime('%Y-%m-%d', time.localtime())
msyyc marked this conversation as resolved.
Show resolved Hide resolved
parameters["client_name"] = code_model.options["client_name"]
parameters["package_pprint_name"] = code_model.options["package_pprint_name"]
parameters['output_folder'] = code_model.options["output_folder"]
parameters['folder_parent'] = Path(parameters["output_folder"]).parts[-2]
parameters['input_file'] = code_model.options["input_file"]
parameters['credential_scopes'] = code_model.options["credential_scopes"]
parameters['swagger_readme_output'] = '../' + parameters["package_name"].replace('-', '/')


return parameters

class JinjaSerializer:
def __init__(self, autorestapi: AutorestAPI) -> None:
self._autorestapi = autorestapi
Expand Down Expand Up @@ -72,6 +98,26 @@ def serialize(self, code_model: CodeModel) -> None:
self._serialize_and_write_models_folder(code_model=code_model, env=env, namespace_path=namespace_path)


if code_model.options["package_mode"] == 'dataplane':
msyyc marked this conversation as resolved.
Show resolved Hide resolved
parameters = _build_package_render_parameters(code_model)
self._serialize_and_write_package_folder(
namespace_path=Path("."),
template_path="templates/templates_package_dataplane",
parameters=parameters
)


def _serialize_and_write_package_folder(
self, namespace_path: Path, template_path: str, parameters: Any
) -> None:
env = _get_environment(template_path)
for template_name in env.list_templates():
template = env.get_template(template_name)
result = template.render(**parameters)
output_name = template_name.replace(".jinja2", "")
self._autorestapi.write_file(namespace_path / output_name, result)



def _keep_patch_file(self, path_file: Path, env: Environment):
if self._autorestapi.read_file(path_file):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Release History
Copy link
Contributor

@iscai-msft iscai-msft Feb 1, 2022

Choose a reason for hiding this comment

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

I think pretty much all of the files you've put down as specific to dataplane package mode can be shared across different packaging modes. All of the differences between --azure-arm and dataplane can be easily resolved in one file: we don't need duplicate templates for each different packaging mode. Not as important, but. wealso really don't need a separate folder for templates_package_dataplane either


## 1.0.0b1 ({{ date_to_release }})

- Initial version

Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
Copyright (c) Microsoft Corporation.

MIT License

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:

The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.

THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
SOFTWARE.
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
include *.md
include {{ folder_first }}/__init__.py
include {{ folder_first }}/{{ folder_second }}/__init__.py
include LICENSE
recursive-include tests *.py
recursive-include samples *.py *.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# {{ package_pprint_name }} client library for Python
<!-- write necessary description of service -->

## _Disclaimer_

_Azure SDK Python packages support for Python 2.7 has ended 01 January 2022. For more information and questions,
please refer to https://github.com/Azure/azure-sdk-for-python/issues/20691_

## Getting started

### Installating the package

```bash
python -m pip install {{ package_name }}
```

#### Prequisites

- Python 3.7+ is required to use this package.
- You need an [Azure subscription][azure_sub] to use this package.
- An existing {{ package_pprint_name }} instance.

#### Create with an Azure Active Directory Credential
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't be sure the package uses AAD, you can look at the credential scope policy property / other credential properties on the code model to see what kind of credentials we use

To use an [Azure Active Directory (AAD) token credential][authenticate_with_token],
provide an instance of the desired credential type obtained from the
[azure-identity][azure_identity_credentials] library.

To authenticate with AAD, you must first [pip][pip] install [`azure-identity`][azure_identity_pip]

After setup, you can choose which type of [credential][azure_identity_credentials] from azure.identity to use.
As an example, [DefaultAzureCredential][default_azure_credential] can be used to authenticate the client:

Set the values of the client ID, tenant ID, and client secret of the AAD application as environment variables:
`AZURE_CLIENT_ID`, `AZURE_TENANT_ID`, `AZURE_CLIENT_SECRET`

Use the returned token credential to authenticate the client:

```python
>>> from {{ namespace }} import {{ client_name }}
>>> from azure.identity import DefaultAzureCredential
>>> client = {{ client_name }}(endpoint='<endpoint>', credential=DefaultAzureCredential())
```

## Examples

```python
>>> from {{ namespace }} import {{ client_name }}
>>> from azure.identity import DefaultAzureCredential
>>> from azure.core.exceptions import HttpResponseError

>>> client = {{ client_name }}(endpoint='<endpoint>', credential=DefaultAzureCredential())
>>> try:
<!-- wirte test code here -->
except HttpResponseError as e:
print('service responds error: {}'.format(e.response.json()))

```

## Next steps

More examples are coming soon...

## Contributing

This project welcomes contributions and suggestions. Most contributions require
you to agree to a Contributor License Agreement (CLA) declaring that you have
the right to, and actually do, grant us the rights to use your contribution.
For details, visit https://cla.microsoft.com.

When you submit a pull request, a CLA-bot will automatically determine whether
you need to provide a CLA and decorate the PR appropriately (e.g., label,
comment). Simply follow the instructions provided by the bot. You will only
need to do this once across all repos using our CLA.

This project has adopted the
[Microsoft Open Source Code of Conduct][code_of_conduct]. For more information,
see the Code of Conduct FAQ or contact opencode@microsoft.com with any
additional questions or comments.

<!-- LINKS -->
[code_of_conduct]: https://opensource.microsoft.com/codeofconduct/
[authenticate_with_token]: https://docs.microsoft.com/azure/cognitive-services/authentication?tabs=powershell#authenticate-with-an-authentication-token
[azure_identity_credentials]: https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/identity/azure-identity#credentials
[azure_identity_pip]: https://pypi.org/project/azure-identity/
[default_azure_credential]: https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/identity/azure-identity#defaultazurecredential
[pip]: https://pypi.org/project/pip/
[azure_sub]: https://azure.microsoft.com/free/
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
-e ../../../tools/azure-devtools
-e ../../../tools/azure-sdk-tools
../../core/azure-core
../../identity/azure-identity
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 know if we need identity. Also we don't need to do 3.x checks anymore, I think by the time we merge this pr, we will have dropped 2.x support in autorest

aiohttp>=3.0
typing_extensions>=3.7.2
asyncio
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# coding=utf-8
# --------------------------------------------------------------------------
#
# Copyright (c) Microsoft Corporation. All rights reserved.
#
# The MIT License (MIT)
#
# Permission is hereby granted, free of charge, to any person obtaining a copy
# of this software and associated documentation files (the ""Software""), to
# deal in the Software without restriction, including without limitation the
# rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
# sell copies of the Software, and to permit persons to whom the Software is
# furnished to do so, subject to the following conditions:
#
# The above copyright notice and this permission notice shall be included in
# all copies or substantial portions of the Software.
#
# THE SOFTWARE IS PROVIDED *AS IS*, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
# FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
# IN THE SOFTWARE.
#
# --------------------------------------------------------------------------
import logging
import os

from {{ namespace }} import {{ client_name }}
from azure.identity import DefaultAzureCredential
from azure.core.exceptions import HttpResponseError

logging.basicConfig(level=logging.DEBUG)
LOG = logging.getLogger()

# Set the values of the client ID, tenant ID, and client secret of the AAD application as environment variables:
# AZURE_CLIENT_ID, AZURE_TENANT_ID, AZURE_CLIENT_SECRET, {{ test_prefix.upper() }}_ENDPOINT
try:
endpoint = os.environ["{{ test_prefix.upper() }}_ENDPOINT"]
except KeyError:
LOG.error("Missing environment variable '{{ test_prefix.upper() }}_ENDPOINT' - please set if before running the example")
exit()

# Build a client through AAD
client = {{ client_name }}(credential=DefaultAzureCredential(), endpoint=endpoint)

# write your sample here. For example:
# try:
# result = client.xxx.xx(...)
# print(result)
# except HttpResponseError as e:
# print('Failed to send JSON message: {}'.format(e.response.json()))
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
[packaging]
package_name = "{{ package_name }}"
package_nspkg = "{{ folder_first }}-{{ folder_second }}-nspkg"
package_pprint_name = "{{ package_pprint_name }}"
package_doc_id = ""
is_stable = false
is_arm = false
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
#!/usr/bin/env python

# -------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See License.txt in the project root for
# license information.
# --------------------------------------------------------------------------


import os
import re

from setuptools import setup, find_packages


# Change the PACKAGE_NAME only to change folder and different name
PACKAGE_NAME = "{{ package_name }}"
PACKAGE_PPRINT_NAME = "{{ package_pprint_name }}"

# a-b-c => a/b/c
package_folder_path = PACKAGE_NAME.replace("-", "/")


# Version extraction inspired from 'requests'
with open(os.path.join(package_folder_path, "_version.py"), "r") as fd:
version = re.search(
r'^VERSION\s*=\s*[\'"]([^\'"]*)[\'"]', fd.read(), re.MULTILINE
).group(1)

if not version:
raise RuntimeError("Cannot find version information")

setup(
name=PACKAGE_NAME,
version=version,
description="Microsoft {} Client Library for Python".format(PACKAGE_PPRINT_NAME),
long_description=open("README.md", "r").read(),
long_description_content_type="text/markdown",
license="MIT License",
author="Microsoft Corporation",
author_email="azpysdkhelp@microsoft.com",
url="https://github.com/Azure/azure-sdk-for-python/tree/main/sdk/{{ folder_parent }}/{{ package_name }}",
classifiers=[
"Development Status :: 4 - Beta",
msyyc marked this conversation as resolved.
Show resolved Hide resolved
"Programming Language :: Python",
"Programming Language :: Python :: 3 :: Only",
"Programming Language :: Python :: 3",
msyyc marked this conversation as resolved.
Show resolved Hide resolved
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
"License :: OSI Approved :: MIT License",
],
zip_safe=False,
packages=find_packages(
exclude=[
# Exclude packages that will be covered by PEP420 or nspkg
"azure",
"azure.{{ folder_second }}",
]
),
install_requires=[
"azure-core<2.0.0,>=1.20.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

since we're setting minversions in multiple setup.py, we should have one common area for min versions

Copy link
Member Author

Choose a reason for hiding this comment

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

each mode has its own dependency so I think it is better to put them solely. And we could update the template when new autorest.python is released if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only realy diff in mode dependency is --azure-arm requires azure-mgmt-core, whil dataplane does not. We should really share dependency requirements for evyerthing else

"msrest>=0.6.21",
'six>=1.11.0',
],
python_requires=">=3.7",
)
Loading