Enable Format Flags for List#105
Conversation
Signed-off-by: kyzrfranz <hieber.thomas@gmx.de>
…king configured devices default Signed-off-by: kyzrfranz <hieber.thomas@gmx.de>
Signed-off-by: kyzrfranz <hieber.thomas@gmx.de>
sabaini
left a comment
sabaini
left a comment
There was a problem hiding this comment.
Hey, thanks @kyzrfranz for this work, looks great!
Some minor nits here and below.
| "github.com/canonical/microceph/microceph/api/types" | ||
| tests2 "github.com/canonical/microceph/microceph/tests" | ||
| "github.com/stretchr/testify/mock" | ||
| "testing" |
There was a problem hiding this comment.
We use this convention for import order:
- built-in
- external imports, e.g. stretchr
- internal imports, e.g. github.com/canonical/microceph/XYZ
| ) | ||
|
|
||
| // make sure it fails like before on empty config | ||
| func Test_cmdDisableRGW_Execute(t *testing.T) { |
There was a problem hiding this comment.
Please use MixedCaps also for the test names, i.e. TestCmdDisableRGWExecute() for this and the below test names
| "github.com/canonical/microceph/microceph/api/types" | ||
| tests2 "github.com/canonical/microceph/microceph/tests" | ||
| "github.com/stretchr/testify/mock" | ||
| "testing" |
There was a problem hiding this comment.
Cf. import ordering comment above
| ) | ||
|
|
||
| // make sure it fails like before on empty config | ||
| func Test_cmdDiskAdd_Execute(t *testing.T) { |
There was a problem hiding this comment.
Please use MixedCaps also for the test names
| microCli "github.com/canonical/microcluster/client" | ||
| "github.com/canonical/microcluster/microcluster" | ||
| "github.com/lxc/lxd/lxc/utils" | ||
| "github.com/lxc/lxd/shared/units" |
There was a problem hiding this comment.
Cf. import ordering comment above
| "github.com/canonical/microceph/microceph/api/types" | ||
| tests2 "github.com/canonical/microceph/microceph/tests" | ||
| "github.com/stretchr/testify/mock" | ||
| "testing" |
There was a problem hiding this comment.
Cf. import ordering comment above
| ) | ||
|
|
||
| // make sure it fails like before on empty config | ||
| func Test_cmdEnableRGW_Execute(t *testing.T) { |
There was a problem hiding this comment.
Please use MixedCase naming
| import ( | ||
| "context" | ||
| "fmt" | ||
| "github.com/lxc/lxd/lxc/utils" |
There was a problem hiding this comment.
Cf. import ordering comment above
| "context" | ||
| "github.com/canonical/microceph/microceph/api/types" | ||
| "github.com/lxc/lxd/shared/api" | ||
| "github.com/stretchr/testify/mock" |
There was a problem hiding this comment.
Please use MixedCase naming
| mock.Mock | ||
| } | ||
|
|
||
| func (m ApiMock) GetDisks(ctx context.Context) (types.Disks, error) { |
There was a problem hiding this comment.
go vet tells me for this and similar below:
GetDisks passes lock by value: github.com/canonical/microceph/microceph/tests.ApiMock contains github.com/stretchr/testify/mock.Mock contains sync.Mutex
Would it be possible to use ApiMock by-ref instead?
|
Hope i got 'em all - realized that theres actually a |
kyzrfranz
force-pushed
the
main
branch
2 times, most recently
from
a39c402 to
3b54e4e
Compare
|
I'm +1 on this; @stgraber @ChrisMacNaughton would you be able to cast an eye? |
|
Change makes sense overall I think, would just like to see this rebased so that each commit makes sense on its own (mostly getting rid of that last fixup comment). |
Signed-off-by: kyzrfranz <hieber.thomas@gmx.de> fix cases & imports Signed-off-by: kyzrfranz <hieber.thomas@gmx.de> fix more cases & imports Signed-off-by: kyzrfranz <hieber.thomas@gmx.de> fix more cases & imports Signed-off-by: kyzrfranz <hieber.thomas@gmx.de> fix more cases & imports Signed-off-by: kyzrfranz <hieber.thomas@gmx.de>
|
@kyzrfranz unfortunately, this needs a rebase again. While you're at it - please take a moment to fix that last commit message. It has what appears to be repeat messges/comments that are unnecessary. |
|
@kyzrfranz Could you please take a look at this PR ? We need another rebase. |
|
Closed due to long inactivity. |
5 participants
Sorry for the confusion - had to resolve some conflicts and did something weird, so #102 got closed :D
Changes as described in #102
So I went ahead and did as mentioned before.
I am not very much in love with how the asserts work on the table tbh but that's due to the fact that I would like to propose some changes to utils.RenderTable first. This should really set up differently tbh only returning the model. You can run some decent assertions on that one, without digging into os.stdout and parsing strings...
So pending that discussion I went for a "works for now" (hopefully) approach.
Let me know how that works for you - I added tests for all commands I can stick the newly proposed ApiClient in.