Skip to content
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.

Sign up for GitHub

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

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
34ccdcd
fix: "device update --lock" should also unlock
displague Aug 26, 2024
bef3873
chore(device update): use Cobra to enforce exclusive fields and no ad…
displague Aug 26, 2024
67dbcd0
docs: revise device update --locked arg
displague Aug 26, 2024
f304a91
chore: refactor device update locked to simple bool
displague Aug 26, 2024
c6562a2
test: add e2e test for device update --locked=true|false
displague Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/metal_device_update.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ Updates a device.
Updates the hostname of a device. Updates or adds a description, tags, userdata, custom data, and iPXE settings for an already provisioned device. Can also lock or unlock future changes to the device.

```
metal device update -i <device_id> [-H <hostname>] [-d <description>] [--locked <boolean>] [-t <tags>] [-u <userdata> | --userdata-file <filepath>] [-c <customdata>] [-s <ipxe_script_url>] [--always-pxe=<true|false>] [flags]
metal device update -i <device_id> [-H <hostname>] [-d <description>] [--locked=<true|false>] [-t <tags>] [-u <userdata> | --userdata-file <filepath>] [-c <customdata>] [-s <ipxe_script_url>] [--always-pxe=<true|false>] [flags]
```

### Examples
Expand Down
18 changes: 9 additions & 9 deletions internal/devices/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ import (
func (c *Client) Update() *cobra.Command {
var (
description string
locked bool
userdata string
userdataFile string
hostname string
Expand All @@ -46,7 +45,7 @@ func (c *Client) Update() *cobra.Command {
)
// updateDeviceCmd represents the updateDevice command
updateDeviceCmd := &cobra.Command{
Use: `update -i <device_id> [-H <hostname>] [-d <description>] [--locked <boolean>] [-t <tags>] [-u <userdata> | --userdata-file <filepath>] [-c <customdata>] [-s <ipxe_script_url>] [--always-pxe=<true|false>]`,
Use: `update -i <device_id> [-H <hostname>] [-d <description>] [--locked=<true|false>] [-t <tags>] [-u <userdata> | --userdata-file <filepath>] [-c <customdata>] [-s <ipxe_script_url>] [--always-pxe=<true|false>]`,
Short: "Updates a device.",
Long: "Updates the hostname of a device. Updates or adds a description, tags, userdata, custom data, and iPXE settings for an already provisioned device. Can also lock or unlock future changes to the device.",
Example: ` # Updates the hostname of a device:
Expand All @@ -64,10 +63,6 @@ func (c *Client) Update() *cobra.Command {
deviceUpdate.Description = &description
}

if userdata != "" && userdataFile != "" {
return fmt.Errorf("either userdata or userdata-file should be set")
}

if userdataFile != "" {
userdataRaw, readErr := os.ReadFile(userdataFile)
if readErr != nil {
Expand All @@ -80,7 +75,11 @@ func (c *Client) Update() *cobra.Command {
deviceUpdate.Userdata = &userdata
}

if locked {
if cmd.Flag("locked").Changed {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@displague displague Aug 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ctreatma added in c6562a2

locked, err := cmd.Flags().GetBool("locked")
if err != nil {
return fmt.Errorf("could not parse locked value: %w", err)
}
deviceUpdate.Locked = &locked
}

Expand Down Expand Up @@ -123,12 +122,13 @@ func (c *Client) Update() *cobra.Command {
updateDeviceCmd.Flags().StringVarP(&description, "description", "d", "", "Adds or updates the description for the device.")
updateDeviceCmd.Flags().StringVarP(&userdata, "userdata", "u", "", "Adds or updates the userdata for the device.")
updateDeviceCmd.Flags().StringVarP(&userdataFile, "userdata-file", "", "", "Path to a userdata file for device initialization. Can not be used with --userdata.")
updateDeviceCmd.Flags().BoolVarP(&locked, "locked", "l", false, "Locks or unlocks the device for future changes (<true|false>).")
updateDeviceCmd.Flags().BoolP("locked", "l", false, "Locks or unlocks the device for future changes (<true|false>).")
updateDeviceCmd.Flags().StringSliceVarP(&tags, "tags", "t", []string{}, `Adds or updates the tags for the device --tags="tag1,tag2".`)
updateDeviceCmd.Flags().BoolVarP(&alwaysPXE, "always-pxe", "a", false, "Updates the always_pxe toggle for the device (<true|false>).")
updateDeviceCmd.Flags().StringVarP(&ipxescripturl, "ipxe-script-url", "s", "", "Add or update the URL of the iPXE script.")
updateDeviceCmd.Flags().StringVarP(&customdata, "customdata", "c", "", "Adds or updates custom data to be included with your device's metadata.")
_ = updateDeviceCmd.MarkFlagRequired("id")

updateDeviceCmd.MarkFlagsMutuallyExclusive("userdata", "userdata-file")
Copy link
Member Author

Choose a reason for hiding this comment

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

$ 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.

updateDeviceCmd.Args = cobra.NoArgs
Copy link
Member Author

Choose a reason for hiding this comment

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

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

return updateDeviceCmd
}
19 changes: 18 additions & 1 deletion test/e2e/devices/deviceupdatetest/device_update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,31 @@ func TestCli_Devices_Update(t *testing.T) {
t.Fatal(err)
}
if status == true {
root.SetArgs([]string{subCommand, "update", "-i", device.GetId(), "-H", "metal-cli-update-dev-test", "-d", "This device used for testing"})
root.SetArgs([]string{subCommand, "update", "-i", device.GetId(), "-H", "metal-cli-update-dev-test", "-d", "This device used for testing", "--locked=true"})

out := helper.ExecuteAndCaptureOutput(t, root)

if !strings.Contains(string(out[:]), "metal-cli-update-dev-test") {
t.Error("expected output should include metal-cli-update-dev-test in the out string ")
}
}

root.SetArgs([]string{subCommand, "delete", "-i", device.GetId()})

out := helper.ExecuteAndCaptureOutput(t, root)

if !strings.Contains(string(out[:]), "not delete") {
t.Error("expected output should include 'not delete' in the out string due to device locking")
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

}

root.SetArgs([]string{subCommand, "update", "-i", device.GetId(), "--locked=false"})

out = helper.ExecuteAndCaptureOutput(t, root)

if !strings.Contains(string(out[:]), "metal-cli-update-dev-test") {
t.Error("expected output should include metal-cli-update-dev-test in the out string ")
}

},
},
}
Expand Down
Loading