-
Notifications
You must be signed in to change notification settings - Fork 115
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
Codegen fixes for the Python SDK #639
Conversation
9207f6b
to
efa6707
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question re: warning visibility; LGTM otherwise
if not __name__: | ||
raise TypeError('Missing resource name argument (for URN creation)') | ||
if not isinstance(__name__, str): | ||
raise TypeError('Expected resource name to be a string') | ||
if __opts__ and not isinstance(__opts__, pulumi.ResourceOptions): | ||
if __opts__ is not None: | ||
warnings.warn("explicit use of __opts__ is deprecated, use 'opts' instead", DeprecationWarning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually render these deprecation warnings in the CLI? I thought we had some issue around this...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied what the TF bridge providers do because it seemed weirder to not do what they did in the same situation. We can do something else if it makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems great, then 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to match the TF providers, we should also consider renaming __name__
to resource_name
:
https://github.com/pulumi/pulumi-aws/blob/master/sdk/python/pulumi_aws/athena/database.py#L28
Indeed it seems odd to make only half of this breaking change - feels like we should align or not make any change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't make this other breaking change because it wasn't listed in the original issue, and we therefore didn't know about it.
Is this all the changes we need to make here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukehoban @pgavlin I've updated to include the breaking __name__
-> resource_name
change as well. Please let me know if there are other things we need to put in here ASAP, as we're planning on merging soon.
d5c56bb
to
b431194
Compare
b431194
to
dca06a9
Compare
Fixes #602.