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

[Package mode] add new flag --package-mode #1154

Merged
merged 32 commits into from
Mar 8, 2022

Conversation

msyyc
Copy link
Member

@msyyc msyyc commented Feb 15, 2022

@msyyc msyyc marked this pull request as ready for review February 15, 2022 06:59
autorest/codegen/__init__.py Outdated Show resolved Hide resolved
autorest/codegen/serializers/__init__.py Outdated Show resolved Hide resolved
autorest/codegen/serializers/__init__.py Outdated Show resolved Hide resolved
@iscai-msft
Copy link
Contributor

overall looks much better, thank you @msyyc!

autorest/codegen/__init__.py Show resolved Hide resolved
autorest/codegen/__init__.py Outdated Show resolved Hide resolved
autorest/codegen/serializers/__init__.py Outdated Show resolved Hide resolved
autorest/codegen/templates/setup.py.jinja2 Outdated Show resolved Hide resolved
autorest/codegen/templates/setup.py.jinja2 Outdated Show resolved Hide resolved
autorest/codegen/templates/MANIFEST.in.jinja2 Outdated Show resolved Hide resolved
@msyyc
Copy link
Member Author

msyyc commented Feb 28, 2022

@iscai-msft
(1) I think we could disable py27 CI check now that we have not supported it yet.
(2) azure-report.json is deleted in latest autorest.testserver: https://github.com/Azure/autorest.testserver/pull/358/files. We need a pr to fix it:

'AzureReport': 'azure-report.json',

@msyyc msyyc mentioned this pull request Mar 2, 2022
Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

def getting closer @lmazuel can you take a look, left some comments ccing you. Thanks!

@@ -76,6 +77,12 @@ def _validate_code_model_options(options: Dict[str, Any]) -> None:
"If you want operation files, pass in flag --show-operations"
)

if options["package_mode"] not in ("mgmtplane", "dataplane", None) and not Path(options["package_mode"]).exists():
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't it just be not in ("mgmtplane", "dataplane")? Not sure why None is a valid entry

Copy link
Member Author

Choose a reason for hiding this comment

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

Path(None).exist() will raise exception so we have to avoid that.

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 really do None checks for any other flag, it's still a little weird because it looks like we're making None a valid entry (by including it with mgmtplane and dataplane)

Copy link
Member Author

Choose a reason for hiding this comment

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

I could check None first and it will make the logic more clear

for template_name in package_files:
file = template_name.replace(".jinja2", "")
output_name = out_path / file
if not self._autorestapi.read_file(output_name) or file == 'setup.py':
Copy link
Contributor

Choose a reason for hiding this comment

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

yay looks good, just want to confirm with @lmazuel : are you good with us not overriding any of the files except setup.py during regeneration? This way if, for example, users update the README, we won't override when regenerating

Copy link
Member

Choose a reason for hiding this comment

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

We should override the MANIFEST as well (though it's unlikely it changes, this should not be changed manually by anyone)

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it and will make a override-list. Now it contains 'setup.py' and 'MAINFEST.in'

# {{ package_pprint_name }} client library for Python
<!-- write necessary description of service -->

## _Disclaimer_
Copy link
Contributor

Choose a reason for hiding this comment

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

Can remove the disclaimer, we've already droppd support for 2.7

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it's fair to remove it we're in March already

Copy link
Member Author

@msyyc msyyc Mar 4, 2022

Choose a reason for hiding this comment

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

Got it

### Installating the package

```bash
python -m pip install {{ package_name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

ideally just do pip install {{ package_name }}

Copy link
Member

Choose a reason for hiding this comment

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

That's debatable, we saw people confused because they had several Python installed, and python and pip were not referencing the same python interpreter. I would suggest to talk about venv instead, since this is a more efficient to ensure consistency between python and pip


def __init__(self, *, status: Optional[Union[str, "OperationResultStatus"]] = None, **kwargs):
"""
:keyword status: The status of the request. Possible values include: "Succeeded", "Failed",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also run at least one acceptance test on each package mode package? This way we can know if the package can be run. So for this package, you can run a paging test, and so on and so forth for the other two package mode packages

Copy link
Member Author

Choose a reason for hiding this comment

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

add test:
test_packagemode.py for dataplane,
test_package_mode_mgmt.py for mgmtplane,
test_package_mode_customize.py for customize


```yaml $(package-mode)
package-configuration:
min_python_version: 3.6
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it's a little weird that this test includes package_name as an entry in the custom template, since we already have flag --package-name, maybe we could switch the packge_name customization to a classifier customization or something

Copy link
Member Author

Choose a reason for hiding this comment

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

changed.

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

ok, i think with the following comments, we should be good to merge!

@@ -76,6 +77,12 @@ def _validate_code_model_options(options: Dict[str, Any]) -> None:
"If you want operation files, pass in flag --show-operations"
)

if options["package_mode"] not in ("mgmtplane", "dataplane", None) and not Path(options["package_mode"]).exists():
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 really do None checks for any other flag, it's still a little weird because it looks like we're making None a valid entry (by including it with mgmtplane and dataplane)

{% set author_email = "azpysdkhelp@microsoft.com" %}
{% set url = "https://github.com/Azure/azure-sdk-for-python/tree/main/sdk" %}
{% else %}
version = "{{ code_model.options.get('package_version', '0.0.0') }}"
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @msyyc. with that check thought, we don't have to set a default value when we get package_version from options. I think you set default value here and at least one more place, so can you change those just to code_model.options["package_version"]?

autorest/codegen/templates/setup.py.jinja2 Show resolved Hide resolved
@@ -12,7 +12,7 @@
from setuptools import setup, find_packages


PACKAGE_NAME = "{{ package_name }}"
PACKAGE_NAME = "azure-packagemode-customize"
Copy link
Contributor

Choose a reason for hiding this comment

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

this file should be setup.py.jinja2 because it's formatted as a jinja template. Endings without .jinja2 means "keep as is" inside the custom package-mode folder, so we should be able to handle both

Copy link
Member Author

Choose a reason for hiding this comment

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

this file should be setup.py.jinja2 because it's formatted as a jinja template. Endings without .jinja2 means "keep as is" inside the custom package-mode folder, so we should be able to handle both

sure

Copy link
Member Author

Choose a reason for hiding this comment

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

some comments are same with up and they are resolved

autorest/codegen/__init__.py Show resolved Hide resolved
@@ -93,14 +98,17 @@ def _serialize_and_write_package_files_proc(**kwargs: Any):
for template_name in package_files:
file = template_name.replace(".jinja2", "")
output_name = out_path / file
if not self._autorestapi.read_file(output_name) or file == 'setup.py':
if not self._autorestapi.read_file(output_name) or file in _REGENERATE_FILES:
Copy link
Contributor

Choose a reason for hiding this comment

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

just to confirm: if a user passes any template / file inside --package-mode=my-custom-folder/, that will override any of the templates we have in autorest?

Copy link
Member Author

Choose a reason for hiding this comment

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

If user passes folder, autorest will use those files instead of built-in template files. This logic is to permit to override specific files when rerun autorest.

Copy link
Contributor

@iscai-msft iscai-msft left a comment

Choose a reason for hiding this comment

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

yay looks great @msyyc!

Want to thank you again for dealing with all of the comments and your patience, going to go ahead and merge!

@iscai-msft iscai-msft merged commit c151a1e into Azure:autorestv3 Mar 8, 2022
@msyyc msyyc deleted the package-mode branch March 9, 2022 00:07
iscai-msft added a commit that referenced this pull request Mar 9, 2022
…into inherit_customization

* 'autorestv3' of https://github.com/Azure/autorest.python:
  dpg service-driven-test (#1180)
  prepare for release (#1181)
  [Package mode] add new flag `--package-mode` (#1154)
  hide operation group docs (#1179)
iscai-msft added a commit that referenced this pull request Mar 10, 2022
…into mypy_code

* 'autorestv3' of https://github.com/Azure/autorest.python: (26 commits)
  dpg service-driven-test (#1180)
  prepare for release (#1181)
  [Package mode] add new flag `--package-mode` (#1154)
  hide operation group docs (#1179)
  add coverage var (#1178)
  improve logging (#1177)
  don't reformat query params for dpg next link calls (#1168)
  Update package.json
  Update ChangeLog.md
  default optional const parameter to none (#1171)
  add storage to ci checks (#1176)
  fail on regeneration diff (#1173)
  Drop Python 2.7 (#1175)
  Add secrets test (#1174)
  bump testserver version (#1172)
  make content type keyword only if multiple (#1167)
  add default value info for params to docs (#1164)
  update release tests to hopefully get coverage report green (#1152)
  fix windows: clean the outputFolderUri  (#1135)
  Update CODEOWNERS
  ...
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