Skip to content

Enable Format Flags for List#102

Closed
kyzrfranz wants to merge 0 commit into
canonical:mainfrom
kyzrfranz:main
Closed

Enable Format Flags for List#102
kyzrfranz wants to merge 0 commit into
canonical:mainfrom
kyzrfranz:main

Conversation

@kyzrfranz
Copy link
Copy Markdown

Since automation is one of the things I'm currently working on, I realised that the list commands don't make use of the formatting. I realise that's due to the fact that the output combines two sets of data essentially, that's not quite straightforward to put into one set.

So the suggestion would be to:

  • enable the format flags on list
  • introduce a new flag -a| --available that shows only locally available, unconfigured disks
  • make listing microceph configured devices the default behavior

hope this is feasible.

Copy link
Copy Markdown
Collaborator

@wolsen wolsen left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @kyzrfranz! This PR feels like it should be multiple commits as there's at least 2 distinct changes in here (list available disks, add format flags).

In addition it'd be great to see some test coverage for new functionality.

Comment thread microceph/cmd/microceph/cluster_list.go Outdated

import (
"context"
"github.com/lxc/lxd/shared/i18n"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The order of the imports should be revisited. The convention for the repository is:

built-in

external imports

internal imports (github.com/canonical/microceph/XYZ in this case)

@kyzrfranz
Copy link
Copy Markdown
Author

kyzrfranz commented Mar 21, 2023

Hey, I've been looking into adding tests and it feels like this will cause some refactoring.

The calls microcluster.App() would have to be put inside a PreRunE from my perspective as starters.
Also this should be mockable, so there will be need for a construct that allows for it later on - for now we could leave as is.
I would also recommend of using the localClient to instantiate an ApiClient from the clientpackage.
Then we can just use that to read disks/resources/etc in the commands and mock it nicely.

type cmdDiskList struct {
	common       *CmdControl
	disk         *cmdDisk
	restClient ApiClient
}

So - I could make a suggestion, but it would affect a lot of code.

There may also be a quick & hacky way, but I'm not particularly keen on going there..

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