search

aws-cloudformation / cloudformation-cli-python-plugin

The CloudFormation Provider Development Toolkit Python Plugin allows you to autogenerate Python code based on an input schema.
Apache License 2.0
108 stars 47 forks source link

removing explicit urllib3 pinning #154

Closed PatMyron closed 3 years ago

PatMyron commented 3 years ago

introduced in https://github.com/aws-cloudformation/cloudformation-cli-python-plugin/pull/137/commits/210e80ba797f5bee5b40020885d0e44154ad230b by @jotompki

fixes https://github.com/aws-cloudformation/cloudformation-cli-python-plugin/security/dependabot

PatMyron commented 3 years ago

Could you elaborate on what this invalid_name does?

newer version of pylint wants variable casing to be different. Just ignored since it felt opinionated/unnecessary

Also, why do we need to remove urllib3<1.26 dependency?

urllib3 is already an indirect dependency. The line removed from setup.py was just pinning a maximum version number, but we want newer versions of urllib3 now due to security vulnerabilites

anshikg commented 3 years ago

Could you elaborate on what this invalid_name does?

newer version of pylint wants variable casing to be different. Just ignored since it felt opinionated/unnecessary

Isn't it better to just made const Upper case? It matches the naming convention too.

Also, why do we need to remove urllib3<1.26 dependency?

urllib3 is already an indirect dependency. The line removed from setup.py was just pinning a maximum version number, but we want newer versions of urllib3 now due to security vulnerabilites

Didn't realize, this change looks good.

priyap286 commented 3 years ago

Could you elaborate on what this invalid_name does?

newer version of pylint wants variable casing to be different. Just ignored since it felt opinionated/unnecessary

Isn't it better to just made const Upper case? It matches the naming convention too.

I think we must correct the names of the constants highlighted by pylint before adding invalid-name to the disable list and not add invalid-name to the disable list to encourage future changes to follow this convention.

PatMyron commented 3 years ago

"we are generating error codes that we serialize from how they are named https://github.com/aws-cloudformation/cloudformation-cli-python-plugin/blob/master/src/cloudformation_cli_python_lib/interface.py#L9-L14 correcting the handler error code names would either cause us to rename every one or it would break deserialization on the workflows side" - @jotompki

So I've disabled the pylint check on the file-level instead of the repo-level to encourage future changes to follow that convention