-
Notifications
You must be signed in to change notification settings - Fork 116
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
[python] Rename yaml.Config file_id parameter #1248
Conversation
All of the other SDKs call this parameter `file`, so rename it from `file_id` with deprecated support for the old name.
@@ -219,7 +219,7 @@ class ConfigFile(pulumi.ComponentResource): | |||
Kubernetes resources contained in this ConfigFile. | |||
""" | |||
|
|||
def __init__(self, name, file_id, opts=None, transformations=None, resource_prefix=None): | |||
def __init__(self, name, file, opts=None, transformations=None, resource_prefix=None, file_id=None): |
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.
Technically this will break the following:
cf = ConfigFile("name", file_id="file")
Traceback (most recent call last):
File "main.py", line 10, in <module>
cf = ConfigFile("name", file_id="file")
TypeError: __init__() missing 1 required positional argument: 'file'
Which makes me wonder if we should even bother adding the file_id=None
?
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 you have an alternate suggestion? It seems like that's a bit of an obscure edge case, and it would be nice to have this line up across the SDKs.
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 think we have two options (aside from keeping the name as-is):
-
If we think specifying
file_id
by name is rare enough, and therefore don't care so much about breaking someone, we could just rename it tofile
without any other changes. Any folks referring to it by name would get the error above when they run their program and will have to fix thefile_id
tofile
(or just not specify this param by name). -
We rename
file_id
tofile
and make it optional, in addition to addingfile_id=None
at the end, and include the deprecation warning, and add a runtime check to ensure it is set.def __init__(self, name, file=None, opts=None, transformations=None, resource_prefix=None, file_id=None): # ... if file_id is not None: warnings.warn("explicit use of file_id is deprecated, use 'file' instead", DeprecationWarning) file = file_id if file is None: raise TypeError("Missing file argument")
The benefit is that we wouldn't break anyone, but the downside is the function signature would indicate that
file
is optional, when it really is required.
If we're ok with the minor breaking change, I'd lean towards (1).
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.
Oh, I realized I misunderstood your initial comment, and that this would cause an error in many cases. It's probably better to go with option 2 for now, and maybe we can make it required in a future update.
Updated with option 2. Glad you caught that @justinvp! |
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.
LGTM
Proposed changes
All of the other SDKs call this parameter
file
,so rename it from
file_id
with deprecatedsupport for the old name.
Related issues (optional)
Fix #1247