Repository navigation
[STORY-4371] feat(Organizations): Add scalingo orgs - #1282
annabelle-scalingo wants to merge 9 commits into
Conversation
There was a problem hiding this comment.
nitpick: it could be organizations.go. I am not sure there is an advantage to the short name.
There was a problem hiding this comment.
i've renamed it, thanks!
|
|
||
| var ( | ||
| orgsCommand = cli.Command{ | ||
| Name: "orgs", |
There was a problem hiding this comment.
opinion: Reading the story I see this is what was chosen. My two cents, I would have kept the full name.
There was a problem hiding this comment.
indeed, that's the name that was chosen by Product, I didn't question myself
@aurelien-reeves-scalingo any thoughts about that?
There was a problem hiding this comment.
This has been specified that way by @leo-scalingo. He wanted to keep it that way
There was a problem hiding this comment.
Actually I would be totally ok to have both, orgs and organizations as alias. But indeed in terminal if it doesn't impact too much semantic having orgs available is something I would push for
| format := renderer.Format(c.String("format")) | ||
|
|
||
| var orgsRenderer renderer.Renderer[[]scalingo.Organization] | ||
| switch format { |
There was a problem hiding this comment.
question: will this automatically go for the table, even if it is not specified? (sorry it's been a while I've been in the cli)
There was a problem hiding this comment.
yes, the global --format flag defaults to table in scalingo/main.go
I checked on staging:
|
|
||
| func (r *orgsListRenderer) Render(ctx context.Context) error { | ||
| if len(r.organizations) == 0 { | ||
| fmt.Println("You are not a member of any organization.") |
There was a problem hiding this comment.
nitpick: we have the io package (github.com/Scalingo/cli/io) avaible for messages, it might be better.
| t.Header([]string{"ID", "SLUG", "ROLE"}) | ||
|
|
||
| for _, organization := range r.organizations { | ||
| err := t.Append([]string{organization.ID, organization.Slug, organization.Role}) |
There was a problem hiding this comment.
IIRC we never handle that error, I am not sure it provides a lot of value. I think it would be acceptable to _ = it.
There was a problem hiding this comment.
agreed to simplify, done
| return errors.Wrap(ctx, err, "get Scalingo client") | ||
| } | ||
|
|
||
| return list(ctx, c, r) |
There was a problem hiding this comment.
nitpick: that split of function doesn't provide a lot of value. Except if it is better for unit testing, otherwise I would merge the functions.
There was a problem hiding this comment.
yes, the split is mainly for testing
it lets us inject a mocked OrganizationsService without depending on the global auth configuration
| "github.com/Scalingo/go-scalingo/v11/scalingomock" | ||
| ) | ||
|
|
||
| type fakeRenderer struct { |
There was a problem hiding this comment.
issue: why not using a generated mock here?
There was a problem hiding this comment.
i didn't find generated mocks in the repo for such cases (the only mocks used in tests come from go-scalingo's scalingomock)
i kept the small fakeRenderer in the test for now, but happy to update if you think it's better to generate a Renderer mock
…lingo/cli into STORY-4371/feat/orgs-command
add the
scalingo orgscommand to list organizations the current user belongs todisplay organization ID, slug, and membership role
support both table and JSON output
update
go-scalingoto 11.9.0 to get OrganizationsList (Scalingo/go-scalingo#540)