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 1 commit
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 @@ -27,7 +27,7 @@ metal device update -i <device_id> [-H <hostname>] [-d <description>] [--locked
-H, --hostname string The new hostname of the device.
-i, --id string The UUID of the device.
-s, --ipxe-script-url string Add or update the URL of the iPXE script.
-l, --locked Locks or unlocks the device for future changes (<true|false>).
-l, --locked bools Locks or unlocks the device for future changes (<true|false>). (default [])
-t, --tags strings Adds or updates the tags for the device --tags="tag1,tag2".
-u, --userdata string Adds or updates the userdata for the device.
--userdata-file string Path to a userdata file for device initialization. Can not be used with --userdata.
Expand Down
15 changes: 10 additions & 5 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 Down Expand Up @@ -80,8 +79,15 @@ func (c *Client) Update() *cobra.Command {
deviceUpdate.Userdata = &userdata
}

if locked {
deviceUpdate.Locked = &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().GetBoolSlice("locked")
if err != nil {
return fmt.Errorf("could not parse locked value: %w", err)
}
if len(locked) > 1 {
return fmt.Errorf("parameter locked may only be set once")
}
deviceUpdate.Locked = &locked[0]
}

if len(tags) > 0 {
Expand Down Expand Up @@ -123,12 +129,11 @@ 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().BoolSliceP("locked", "l", []bool{}, "Locks or unlocks the device for future changes (<true|false>).")
ctreatma marked this conversation as resolved.
Show resolved Hide resolved
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")

return updateDeviceCmd
}
Loading