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

Added a method to provide plugin information that is included in artifact zip #167

Merged
merged 9 commits into from
Oct 21, 2021

Conversation

priyap286
Copy link
Contributor

@priyap286 priyap286 commented Sep 30, 2021

Description of changes:
Added a method to provide plugin information that is included in artifact zip.
Plugin information includes,

plugin-tool-version - The version of the cli tool
plugin-name - "java"/"python"/"go". Here it is "python"

I also updated the mypy version and made few other changes based on pre-commit run.

Testing:

  • Unit testing
  • Local testing
less .cfn_metadata.json 
{"plugin-tool-version": "2.1.4", "plugin-name": "python", "cli-version": "0.2.19"}
.cfn_metadata.json (END)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@@ -49,7 +49,7 @@ repos:
- id: bandit
files: ^(src|python)/
- repo: https://github.com/pre-commit/mirrors-mypy
rev: v0.711
rev: v0.790
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Upgraded mypy because of python/mypy#9916.

@@ -14,7 +14,7 @@ def recast_object(
if not isinstance(json_data, dict):
raise InvalidRequest(f"Can only parse dict items, not {type(json_data)}")
# if type is Any, we leave it as is
if cls == typing.Any:
if cls is typing.Any:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made this change because of pylint error,
W0143: Comparing against a callable, did you omit the parenthesis? (comparison-with-callable)

@@ -127,5 +127,5 @@ def get_forward_ref_type() -> Any:
# introspection is valid:
# https://docs.python.org/3/library/typing.html#typing.ForwardRef
if "ForwardRef" in dir(typing):
return typing.ForwardRef # type: ignore
return typing.ForwardRef
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed "# type: ignore" because of mypy complaining of unused 'type: ignore' comment

@@ -169,6 +170,10 @@ def generate(self, project):

LOG.debug("Generate complete")

# pylint: disable=unused-argument
Copy link
Contributor

Choose a reason for hiding this comment

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

if project is unused then why is it in the method? add annotation on what method is returning plz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added comments as to why the argument project is part of the method signature but not used. Link to code in cloudformation-cli, https://github.com/aws-cloudformation/cloudformation-cli/blob/master/src/rpdk/core/project.py#L546

python/rpdk/python/codegen.py Outdated Show resolved Hide resolved
Copy link
Contributor

@omkhegde omkhegde left a comment

Choose a reason for hiding this comment

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

Please make sure this change works locally as well

@priyap286 priyap286 requested a review from ammokhov October 14, 2021 19:03
@priyap286 priyap286 requested a review from ammokhov October 16, 2021 00:38
@@ -15,6 +16,7 @@
from rpdk.core.jsonutils.resolver import ContainerType, resolve_models
from rpdk.core.plugin_base import LanguagePlugin

from . import __version__
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Check the _get_plugin_information() method below. Link

Comment on lines +130 to +132
if sys.version_info < (3, 7):
return typing._ForwardRef # type: ignore
return typing.ForwardRef
Copy link
Contributor Author

@priyap286 priyap286 Oct 21, 2021

Choose a reason for hiding this comment

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

Made these changes because of the following failure, https://github.com/aws-cloudformation/cloudformation-cli-python-plugin/pull/167/checks?check_run_id=3909535606. But in the end because of issues highlighted in,

I still had to use --no-warn-unused-ignores in .pre-commit-config.yaml.

But I believe using sys.version_info is the right way to go about it. (Reference)

I verified from python documentation that ForwardRef was introduced in python 3.7 (source code, link to python 3.6 source code for reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Locally tested it.

@priyap286 priyap286 merged commit 386dbb4 into aws-cloudformation:master Oct 21, 2021
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