-
Notifications
You must be signed in to change notification settings - Fork 134
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
pylxd/models/certificate: re-add password arg for backward compat #603
pylxd/models/certificate: re-add password arg for backward compat #603
Conversation
Wouldn't it make more sense to update charm-lxd instead? Maybe by updating the function call based on the pyLXD version. Maybe there is some limitation there that I am not aware of. |
Fixing charm-lxd would be easy but there are probably other pyLXD users out there we don't want to break, at least not without a compelling reason. |
@simondeziel Oh I see, and that reminds me that the extension does not imply in the password being deprecated, so this would still break users using positional arguments for So I propose using And maybe next release we make An alternative would be using a boolean flag instead of an optional That way we allow for using password auth even with the extension and don't break any users. What do you think? If that is too dev-y for you I can quickly add these changes here later today if you agree they make sense. |
The fact that this method always used If I missed your point, please feel free to suggest a different approach inline in the code review. |
bd6b150
to
1f00d60
Compare
Fixes canonical/charm-lxd#168 where charm-lxd is calling certificates.create(): ```python config: Dict[str, Union[str, bytes, List[str], bool]] = { "name": name, "password": "", "cert_data": cert.encode(), } client.certificates.create(**config) ``` causing: ``` File "./src/charm.py", line 1139, in _on_https_relation_changed if self.lxd_trust_add(cert=cert, name=cert_name, projects=projects): File "./src/charm.py", line 2294, in lxd_trust_add client.certificates.create(**config) TypeError: create() got an unexpected keyword argument 'password' ``` Signed-off-by: Simon Deziel <[email protected]>
3f8ff39
to
6493f49
Compare
@hamistao ready for another look when you have time, thanks! |
Signed-off-by: Simon Deziel <[email protected]>
6493f49
to
d131c01
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.
I am assuming this is ready for review.
If so it looks fine to me, thanks for this!
Fixes canonical/charm-lxd#168 where charm-lxd is calling certificates.create():
causing:
This should fix an unexpected regression introduced in commit ec1b3ee.