Conversation
Mia-Cross
added
instance
a4c8dce to
802d16a
Compare
|
|
||
| cmds.MustFind("instance", "placement-group", "create").Override(placementGroupCreateBuilder) | ||
| cmds.MustFind("instance", "placement-group", "get").Override(placementGroupGetBuilder) | ||
| cmds.MustFind("instance", "placement-group", "list").Override(placementGroupListBuilder) |
There was a problem hiding this comment.
Just as a confirmation, the following (v1 exclusive) fonction:
cmds.MustFind("instance", "placement-group", "get-servers").Override(...)is not present here because the default generated function is enough?
There was a problem hiding this comment.
Yes, this method and the other ones that are not present in v2 (instancePlacementGroupSet(), instancePlacementGroupGetServers(), instancePlacementGroupSetServers() and instancePlacementGroupUpdateServers()), never had any custom logic.
We're not sure if these methods are used or not, so we decided to keep the generated functions and deprecate them instead of just deleting them.
There was a problem hiding this comment.
Ok 👍🏻 Shouldn't the commands be deprecated in internal/namespaces/instance/v1/instance_cli.go then?
There was a problem hiding this comment.
Yes, although the file is generated, we could mark the commands as deprecated at generation level, but since we need to add some custom logic anyway (for the deprecation message), we might as well deprecate the command from here.
| } | ||
|
|
||
| renameOrganizationIDArgSpec(c.ArgSpecs) | ||
| renameProjectIDArgSpec(c.ArgSpecs) |
There was a problem hiding this comment.
Kinda nit: I see in this golden cmd/scw/testdata/test-all-usage-instance-placement-group-usage.golden that we now have an organization and a project attribute. Usual namings seem to be project-id, organization-id. I'm not sure but maybe the disappearance of these lines has something to do with it? Shouldn't these renames be used in the v2 as well?
There was a problem hiding this comment.
Instance v1 API, as it is one of the oldest APIs, had organization and project fields. This custom logic was needed then to be able to use organization-id and project-id on all CLI namespaces, but v2 now has the right fields so we can drop it.
The usage goldens were broken at the time of your review because the namespace is currently a mix of v1 and v2, which confuses the doc generation. I believe the issue is fixed now 👍
72f9a11 to
26c2711
Compare
3 participants
No description provided.