fix: "device update --locked" should also unlock

[displague]

Fixes #484

Previous behavior was to set {"locked":true} in PUT requests whenever --locked was specified. This is because the presence of the flag itself is treated as a toggle. The value argument was ignored unless given as --locked=true or --locked=false. When --locked=false was given, the PUT request was {}, since the code required true values to add the field to the update request.

This change technically breaks the current ability to do metal device update --locked --some-other-arg.

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked false --locked true --id 1234
Error: parameter locked may only be set once

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked --id 1234                    
Error: invalid argument "--id" for "-l, --locked" flag: strconv.ParseBool: parsing "--id": invalid syntax
...

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked true  --id 1234              
PUT /metal/v1//devices/1234 HTTP/1.1
...
{"locked":true}

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked false  --id 1234
PUT /metal/v1//devices/1234 HTTP/1.1
...
{"locked":false}

[displague]

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked false  --id 1234 --userdata foo --userdata-file=/tmp/foo
Error: if any flags in the group [userdata userdata-file] are set none of the others can be; [userdata userdata-file] were all set
Usage:
...

Surprisingly, this does not affect make generate-docs.

[displague]

$ PACKNGO_DEBUG=1 go run ./cmd/metal device update --locked false  --id 1234 fkk fll                                
Error: unknown command "fkk" for "metal device update"

[ctreatma]

Re: the fix! tag, we're at v0 so any change can be breaking regardless of which version number component was bumped. The release action assumes that you don't want to stay v0, so the fix! tag will make the next release of this CLI a v1 release. Is that the intent?

[displague]

Updated the behavior to expect --locked or --locked=<true|false> and revised the docs to match.

Using --locked true|false will result in an error:

Error: unknown command "false" for "metal device update"

[ctreatma]

A test for lock/unlock would build confidence that we've fixed the issue

[displague]

I started down a path of introducing a mock because I don't like the idea of spinning up and settling electrified and cooled rust to verify that a few bytes of command line argument trigger a few bytes of json in an HTTP call.

221430d

The test in the sha is not clean or complete, but it does trigger the lock in the CLI args and look for the request to contain the expected JSON.

Alternatively, I could add steps to the existing "update" E2E to see if the resource locked state can be toggled on and off again (which would be needed for a successful cleanup).

Interested in your thoughts on a preferred path, @ctreatma.

[ctreatma]

I don't have a strong preference either way for whether the test hits a live API or a mock. If we're going to mock I think it's better to set up a mock API than to inject a mock HTTP transport, since that more closely matches real usage, reduces the risk of accidentally mocking out some custom behavior, and provides a more straightforward path to maybe eventually adopting a mock that is generated from the API spec.

[displague]

@ctreatma added in c6562a2

[stevemar]

IIRC, using true/false as allowed values for a flag is generally not typical convention for CLIs. There should be an opposite flag, like --unlock to flip it. (See https://stackoverflow.com/questions/52403065/argparse-optional-boolean)

[displague]

@stevemar
This PR addresses the problem with --locked false (space separated). This problem appears elsewhere in the project, for example, --force false. That will be corrected with #486.

The precedent in the metal-cli is for bool's to have --name=<true|false>.
There's at least one exception that I see:

• --app vs --sms for TFA

We may find other places where --name=false is used, where false is the default but must be explicitly sent in API calls. It's confused to specify and with Cobra, it is easy to miss during code review.

Metal CLI can certainly introduce more obvious patterns in future versions, such as named pairs (lock, unlock) or --no- prefixes. It would be great to get a better bool toggle behavior from the upstream project(s) too.

Related issues:

• Please allow booleans to be set false with no- prefix spf13/cobra#958
• Support Boolean flags with only NoOptDefVal, no --foo=true syntax. spf13/pflag#214

Another alternative that fits this particular parameter would be to introduce lock and unlock metal device actions: metal device lock --id 1234 vs metal device update --lock --id 1234.

metal port convert is a complicated example of this. It introduces a separate action, rather than metal port update --layer-2, but it also uses a parameter to trigger a secondary action: metal port convert --bond 🙃.

The complications owe to RPC-like port endpoints in the otherwise REST-y API. In REST API's the verb is the HTTP Action (GET, PUT, DELETE ...), it should not be in the path name (/convert, /assign, /disbond).

Metal CLI could be more predictable about how it approaches API to CLI translation, if not dogmatic. Documentation in CONTRIBUTING.md or #106 could help with that.

---

I created #487 from this feedback and to continue this conversation.

[ctreatma]

This test is failing here; haven't checked this PR out yet to get a better idea of which part of this is failing (Device is not locked? Device is locked but the error message doesn't match the check on line 58? Something else?).

https://github.com/equinix/metal-cli/actions/runs/10741510710/job/29792085636?pr=485#step:5:77

[ctreatma]

The delete subcommand waits for confirmation before deleting: https://github.com/equinix/metal-cli/blob/main/internal/devices/delete.go#L40-L41

This test will need to delete with the -f flag to bypass the confirmation prompt.