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.

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

Rework device subcommand and client APIs to support lookup by UUID #414

Merged
merged 27 commits into from
Jun 10, 2024

Conversation

doanac
Copy link
Member

@doanac doanac commented Jun 5, 2024

This series of changes ultimately moves fioctl to support device commands where
they can be addressed by UUID instead of name. e.g.:

fioctl devices show --by-uuid xxx-xxx-xxxx
fioctl devices config log -n1 -u xxx-xxx-xxxx

To get there I cleaned up the client APIs to make this more organized and support
a "device api". This separates a Device object - which requires a device lookup
from a DeviceApi which can make API calls w/o looking up a device.

Just part of our long term goal of making client.go smaller

Signed-off-by: Andy Doan <[email protected]>
This will allow us to later on make our CLI work with either UUID or names.
It also makes the APIs a little more segregated and easier to read.

Signed-off-by: Andy Doan <[email protected]>
@doanac doanac requested a review from vkhoroz June 5, 2024 17:26
Copy link
Member

@vkhoroz vkhoroz left a comment

Choose a reason for hiding this comment

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

Overall, good job.

It is a bit extra-ordinary, but in the end a really clever way to implement this.

My only comments are called to make the commit chain more refined.
Why? It was a pleasure to review this up until the last 2 commits...
...Up until that point I had a feeling that everything evolves gradually, hence is under control.
...The one before last commit is a kind of an overhaul of these nice intermediate commits which IMHO could be avoided by changing the commit order.

subcommands/devices/config_log.go Outdated Show resolved Hide resolved
subcommands/devices/cmd.go Show resolved Hide resolved
subcommands/devices/cmd.go Outdated Show resolved Hide resolved
subcommands/devices/cmd.go Outdated Show resolved Hide resolved
subcommands/devices/show.go Show resolved Hide resolved
@doanac
Copy link
Member Author

doanac commented Jun 6, 2024

Why? It was a pleasure to review this up until the last 2 commits...

Ha - you caught me. I had a completely clear vision of the clean ups and then on the last two was sort of feeling my way through the dark. I'll clean that up.

doanac added 17 commits June 6, 2024 14:26
We deprecated this in 2021. Should be safe now.

Signed-off-by: Andy Doan <[email protected]>
The commits following this will allow showing a device using its device
 UUID. This means the name attribute will become important.

Signed-off-by: Andy Doan <[email protected]>
The changes that follow this will need a consistent way to determine the
device it should target its operations against. This change makes a
single place to look the device up from.

Signed-off-by: Andy Doan <[email protected]>
Copy link
Member

@vkhoroz vkhoroz left a comment

Choose a reason for hiding this comment

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

LGTM

This looks nice.

subcommands/devices/cmd.go Show resolved Hide resolved
subcommands/devices/cmd.go Outdated Show resolved Hide resolved
Copy link
Member

@vkhoroz vkhoroz left a comment

Choose a reason for hiding this comment

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

This is good!

@doanac doanac merged commit 9662e81 into foundriesio:main Jun 10, 2024
8 checks passed
@doanac doanac deleted the device-uuid branch June 10, 2024 16:35
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.

2 participants