Add optional property "cloud_info" for VMIs #637
Conversation
JAVGan
requested review from
rohanpm,
rbikar and
chandwanitulsi
as code owners
|
LGTM |
src/pushsource/_impl/model/vms.py
Outdated
| default=None, | ||
| validator=optional((instance_of(VMICloudInformation))), | ||
| ) | ||
| """Cloud provider information of its short name and account alias.""" |
There was a problem hiding this comment.
I would prefer that this is worded in such a way that it doesn't imply these are the only attributes.
For example, instead of "of its" it could be something like:
| """Cloud provider information of its short name and account alias.""" | |
| """Cloud provider information, such as the provider's short name and account alias.""" |
"such as" communicating that these are only examples and you should check the class docs if you want to know everything.
The reason for this is that, if someone later needs to add more attributes to VMICloudInformation, I think it's likely they will not come back and update this doc string. The doc string therefore would be best worded with future additions in mind to avoid becoming inaccurate.
src/pushsource/_impl/model/vms.py
Outdated
| @@ -24,6 +24,17 @@ def __repr__(self): | |||
| return f"{cls_name}.{self.name}" | |||
|
|
|||
|
|
|||
| @attr.s() | |||
| class VMICloudInformation(object): | |||
There was a problem hiding this comment.
Could you please use "Info" rather than "Information"?
There are already classes in this library using "Info" (KojiBuildInfo, ContainerImagePullInfo) so we should maintain consistency.
There was a problem hiding this comment.
Fixed, thanks for reminding me about that!
src/pushsource/_impl/model/vms.py
Outdated
| @@ -24,6 +24,17 @@ def __repr__(self): | |||
| return f"{cls_name}.{self.name}" | |||
|
|
|||
|
|
|||
| @attr.s() | |||
| class VMICloudInformation(object): | |||
| """Inform the cloud provider's name and account alias of a given push item.""" | |||
There was a problem hiding this comment.
Overall, I find the intended usage of it a bit unclear and would appreciate a more detailed doc string.
Specifically I find it unclear if this is meant to be used as:
- the cloud provider which should be used to publish the item, or:
- the cloud provider on which the item currently exists, or:
- both
The PR cover message talks about using this for "delete", which sounds like case (2) as you're going to use this to look up the image in a given provider. If it's indeed case (2) then this all looks good except that I request the doc string is fleshed out a bit with a sentence or two to explain the expected usage.
This is the kind of doc string I think we should have (and I'm assuming case 2 when writing this):
Information on the cloud provider associated with a given push item.
Cloud provider information is only available for VMIs which have previously
been published to a cloud. It may be used to locate an existing VMI for
manipulation, such as metadata updates or deletion.
This library doesn't define any specific cloud provider names or aliases.
Generally, a user of this library is expected to use the information here
to look up cloud access details from a configuration file or other source.
If the case is (1) or (3), as well as expanding the doc string, could we please rename the attribute to dest_cloud_info ?
The reason behind this is that I am concerned about the push item attributes becoming a confusing mix between attributes having the current state of some content, vs desired state. For example: there would be an attribute named build_info saying where the push item came from, and then a similarly named cloud_info saying where the push item should be pushed. There's no way a developer could be reasonably expected to remember which of these attributes represent the current vs desired state.
The last time this came up was ContainerImagePushItem.dest_signing_key, representing a signing key that an image should be signed with (not "is already signed with"), so the established convention is dest_*.
There was a problem hiding this comment.
Our goal with this new optional property is really just for the case 2.
I've adjusted the docstring accordingly, thanks a lot for the suggestion, I really appreciate it. I've set it with the same value as it correctly represent the goal of this class.
This commit introduces a new optional property for all VMIPushItems named `VMICloudInformation` which stores the cloud's provider name and account alias into it. The goal of this change is to be able to parse the patched `clouds.json` with this new property, which was introduced in release-engineering/pubtools-marketplacesvm#71, allowing the DeleteTask to have a more reliable way to retrieve the provider/account information. Refers to SPSTRAT-363
This commit introduces a new optional property for all VMIPushItems named
VMICloudInformationwhich stores the cloud's provider name and account alias into it.The goal of this change is to be able to parse the patched
clouds.jsonwith this new property, which was introduced inrelease-engineering/pubtools-marketplacesvm#71 , allowing the DeleteTask to have a more reliable way to retrieve the provider/account information.
Refers to SPSTRAT-363