Skip to content

Conversation

Dorthu
Copy link
Collaborator

@Dorthu Dorthu commented Apr 26, 2021

In testing an unrelated CLI change, I noticed this:

$ linode-cli linodes create --help
linode-cli linodes create
Linode Create

Arguments:
  --root_pass: This will set the root user's password on the newly-created Linode.
  --stackscript_id: A StackScript ID that will cause the referenced StackScript to be run duringdeployment of this Linode.
  --authorized_keys: A list of public SSH keys that will be automatically appendedto the root user's `~/.
  --authorized_users: A list of usernames.
  --stackscript_data: This field is required only if the StackScript being deployed requires inputdata from the User for successful completion.
  --booted: This field defaults to `true` if the Linode is created with an Image or from a Backup.
  --image: An Image ID to deploy the Disk from.

That's not good - the linodes create operation is missing many
parameters, including type and region that are required. After some
digging, I determined that it was because the allOf in
/linodes/instances.post.requestBody.content.application/json.schema
section was not nested properly; the schema should be all of the
references LinodeRequest and the given properties. Having an
allOf alongside properties isn't really correct (although it's
apparently not invalid).

Built off this spec, the CLI now see the full request:

$ python -m linodecli linodes create --help
linode-cli linodes create
Linode Create

Arguments:
  --region: (required) The [Region](/docs/api/regions/#regions-list) where the Linode will be located.
  --type: (required) The [Linode Type](/docs/api/linode-types/#types-list) of the Linode you are creating.
  --interfaces.label: The name of this interface.
  --root_pass: This will set the root user's password on the newly-created Linode.
  --backup_id: A Backup ID from another Linode's available backups.
  --group: A deprecated property denoting a group label for this Linode.
  --stackscript_id: A StackScript ID that will cause the referenced StackScript to be run duringdeployment of this Linode.
  --authorized_keys: A list of public SSH keys that will be automatically appendedto the root user's `~/.
  --stackscript_data: This field is required only if the StackScript being deployed requires inputdata from the User for successful completion.
  --image: An Image ID to deploy the Disk from.
  --tags: An array of tags applied to this object.
  --backups_enabled: If this field is set to `true`, the created Linode will automatically beenrolled in the Linode Backup service.
  --label: The Linode's label is for display purposes only.
  --interfaces.purpose: The type of interface.
  --private_ip: If true, the created Linode will have private networking enabled and assigned a private IPv4 address.
  --swap_size: When deploying from an Image, this field is optional, otherwise it is ignored.
  --interfaces.ipam_address: This Network Interface's private IP address in Classless Inter-Domain Routing (CIDR) notation.
  --authorized_users: A list of usernames.
  --booted: This field defaults to `true` if the Linode is created with an Image or from a Backup.

I could be persuaded that the CLI's spec parser is incorrectly merging
the allOf with the same-level properties, but I do not believe this
is the case. The OpenAPI Specification is light on details here, but
all examples using allOf do not include sibling nodes like this.

In testing an unrelated CLI change, I noticed this:

```bash
$ linode-cli linodes create --help
linode-cli linodes create
Linode Create

Arguments:
  --root_pass: This will set the root user's password on the newly-created Linode.
  --stackscript_id: A StackScript ID that will cause the referenced StackScript to be run duringdeployment of this Linode.
  --authorized_keys: A list of public SSH keys that will be automatically appendedto the root user's `~/.
  --authorized_users: A list of usernames.
  --stackscript_data: This field is required only if the StackScript being deployed requires inputdata from the User for successful completion.
  --booted: This field defaults to `true` if the Linode is created with an Image or from a Backup.
  --image: An Image ID to deploy the Disk from.
```

That's not good - the `linodes create` operation is missing many
parameters, including `type` and `region` that are required.  After some
digging, I determined that it was because the `allOf` in
`/linodes/instances.post.requestBody.content.application/json.schema`
section was not nested properly; the schema should be all of the
references `LinodeRequest` _and_ the given properties.  Having an
`allOf` alongside `properties` isn't really correct (although it's
apparently not invalid).

Built off this spec, the CLI now see the full request:

```bash
$ python -m linodecli linodes create --help
linode-cli linodes create
Linode Create

Arguments:
  --region: (required) The [Region](/docs/api/regions/#regions-list) where the Linode will be located.
  --type: (required) The [Linode Type](/docs/api/linode-types/#types-list) of the Linode you are creating.
  --interfaces.label: The name of this interface.
  --root_pass: This will set the root user's password on the newly-created Linode.
  --backup_id: A Backup ID from another Linode's available backups.
  --group: A deprecated property denoting a group label for this Linode.
  --stackscript_id: A StackScript ID that will cause the referenced StackScript to be run duringdeployment of this Linode.
  --authorized_keys: A list of public SSH keys that will be automatically appendedto the root user's `~/.
  --stackscript_data: This field is required only if the StackScript being deployed requires inputdata from the User for successful completion.
  --image: An Image ID to deploy the Disk from.
  --tags: An array of tags applied to this object.
  --backups_enabled: If this field is set to `true`, the created Linode will automatically beenrolled in the Linode Backup service.
  --label: The Linode's label is for display purposes only.
  --interfaces.purpose: The type of interface.
  --private_ip: If true, the created Linode will have private networking enabled and assigned a private IPv4 address.
  --swap_size: When deploying from an Image, this field is optional, otherwise it is ignored.
  --interfaces.ipam_address: This Network Interface's private IP address in Classless Inter-Domain Routing (CIDR) notation.
  --authorized_users: A list of usernames.
  --booted: This field defaults to `true` if the Linode is created with an Image or from a Backup.
```

I could be persuaded that the CLI's spec parser is incorrectly merging
the `allOf` with the same-level `properties`, but I do not believe this
is the case.  The OpenAPI Specification is light on details here, but
all examples using `allOf` do not include sibling nodes like this.
@Dorthu Dorthu changed the base branch from development to master April 26, 2021 13:46
@Dorthu Dorthu changed the title bugfix/fix linode cli linodes create Fix linode-cli linodes create operation Apr 26, 2021
@Dorthu
Copy link
Collaborator Author

Dorthu commented Apr 26, 2021

I pointed this at master since the CLI's linode create operation is presently broken - I could point it back at development if you prefer.

@leslitagordita leslitagordita self-requested a review April 27, 2021 20:38
Copy link
Contributor

@leslitagordita leslitagordita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbiggerr I pulled this down and built the site locally and it lgtm. @Dorthu has requested that we get this out with tomorrow's release.

@bbiggerr bbiggerr merged commit 36708c7 into linode:master Apr 28, 2021
LBGarber pushed a commit to LBGarber/linode-api-docs that referenced this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants