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

cli keeps calling non paginated endpoint even when interactive flag is on #847

Merged
merged 8 commits into from
Nov 2, 2022
Merged

cli keeps calling non paginated endpoint even when interactive flag is on #847

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
345bf1f
call workspace pagination query when interactive flag set to true
ajayy004 Oct 31, 2022
c625e3b
fixed lint issue
ajayy004 Oct 31, 2022
991f836
* changed switchToLastUsedWorkspace function to query workspace inste…
ajayy004 Nov 1, 2022
e66089b
replaced getWorkspace query with ValidateWorkspaceId to avoid pulling…
ajayy004 Nov 1, 2022
b81318a
fixed lint issue
ajayy004 Nov 1, 2022
072528c
added test coverage for ValidateWorkspaceID
ajayy004 Nov 1, 2022
d37ee5a
added test coverage for switchToLastUsedWorkspace
ajayy004 Nov 1, 2022
807b40f
added comment, why we need call with certain workspace page size to i…
ajayy004 Nov 2, 2022
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
16 changes: 15 additions & 1 deletion software/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,20 @@ func registryAuth(client houston.ClientInterface, out io.Writer) error {
return nil
}

func getWorkspaces(client houston.ClientInterface, interactive bool) ([]houston.Workspace, error) {
var workspaces []houston.Workspace
var err error

if interactive {
workspacePageSize := 2
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use the PageSize value from config.CFG.PageSize or else defaultPageSize?
Reference: https://github.com/astronomer/astro-cli/blob/main/software/auth/auth.go#L135

Copy link
Contributor Author

@ajayy004 ajayy004 Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Since we need to identify if the number of workspaces returned 1 or more, calling workspace pagination with page size 2 if the interactive flag is set to true, pulling 100 records does not make sense as we just need to make a decision of 1 or more.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's not entirely true, because the list of workspaces returned from this method is used to switch to last_used_workspace, and by just pulling two workspaces, that functionality will almost always be broken in case interactive is set to true.

So from a UX point of view, if interactive is set to true, then whenever a user tries to log in, they would be asked to select a particular workspace every single time, and not automatically set the last used workspace from the previous login as the current workspace.

We should document this in our release note if we are going with this approach, and also add comments in the code around the logic of choosing the page size as 2 for future reference.

Might want to check once on this functional change with @cmarteepants if not already done.

Copy link
Contributor

Choose a reason for hiding this comment

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

The trade-off (selecting the workspace every single time a user logs in) is too expensive from a UX standpoint. I'm not following how the change helps solve the initial problem. Is there another approach we can use to solve the problem, while retaining the login UX?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I'll change the implementation of switchToLastUsedWorkspace to use getWorkspace API to identify if the user still has access/available last_used_workspace workspace. That way will have the same user experience.

ajayy004 marked this conversation as resolved.
Show resolved Hide resolved
workspaces, err = client.PaginatedListWorkspaces(workspacePageSize, 0)
} else {
workspaces, err = client.ListWorkspaces()
}

return workspaces, err
}

// Login handles authentication to houston and registry
func Login(domain string, oAuthOnly bool, username, password string, client houston.ClientInterface, out io.Writer) error {
var token string
Expand Down Expand Up @@ -173,7 +187,7 @@ func Login(domain string, oAuthOnly bool, username, password string, client hous
return err
}

workspaces, err := client.ListWorkspaces()
workspaces, err := getWorkspaces(client, interactive)
if err != nil {
return err
}
Expand Down
71 changes: 61 additions & 10 deletions software/auth/auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,38 @@ func TestLoginSuccess(t *testing.T) {
houstonMock.AssertExpectations(t)
})

t.Run("with workspace pagination switch", func(t *testing.T) {
t.Run("default with more than one workspace", func(t *testing.T) {
fs := afero.NewMemMapFs()
configYaml := testUtil.NewTestConfig("localhost")
afero.WriteFile(fs, config.HomeConfigFile, configYaml, 0o777)
config.InitConfig(fs)
config.CFG.Interactive.SetHomeString("false")
config.CFG.PageSize.SetHomeString("100")

defer testUtil.MockUserInput(t, "1")()

houstonMock := new(houstonMocks.ClientInterface)
houstonMock.On("GetAuthConfig", mock.Anything).Return(&houston.AuthConfig{LocalEnabled: true}, nil)
houstonMock.On("AuthenticateWithBasicAuth", mock.Anything, mock.Anything, mock.Anything).Return(mockToken, nil)
houstonMock.On("ListWorkspaces").Return([]houston.Workspace{{ID: "ck05r3bor07h40d02y2hw4n4v"}, {ID: "test-workspace-id"}}, nil).Twice()
switchToLastUsedWorkspace = func(c *config.Context, workspaces []houston.Workspace) bool {
return false
}
houstonMock.On("GetWorkspace", "ck05r3bor07h40d02y2hw4n4v").Return(&houston.Workspace{}, nil).Once()

out := &bytes.Buffer{}
if err := Login("localhost", false, "test", "test", houstonMock, out); (err != nil) != false {
t.Errorf("Login() error = %v, wantErr %v", err, false)
return
}
if gotOut := out.String(); !testUtil.StringContains([]string{"localhost", "test-workspace-id"}, gotOut) {
t.Errorf("Login() = %v, want %v", gotOut, []string{"localhost", "test-workspace-id"})
}

houstonMock.AssertExpectations(t)
})

t.Run("when interactive set to true, auto selected first workspace if returned one workspace", func(t *testing.T) {
fs := afero.NewMemMapFs()
configYaml := testUtil.NewTestConfig("localhost")
afero.WriteFile(fs, config.HomeConfigFile, configYaml, 0o777)
Expand All @@ -274,7 +305,7 @@ func TestLoginSuccess(t *testing.T) {
houstonMock := new(houstonMocks.ClientInterface)
houstonMock.On("GetAuthConfig", mock.Anything).Return(&houston.AuthConfig{LocalEnabled: true}, nil)
houstonMock.On("AuthenticateWithBasicAuth", mock.Anything, mock.Anything, mock.Anything).Return(mockToken, nil)
houstonMock.On("ListWorkspaces").Return([]houston.Workspace{{ID: "test-workspace-id"}}, nil).Once()
houstonMock.On("PaginatedListWorkspaces", 2, 0).Return([]houston.Workspace{{ID: "ck05r3bor07h40d02y2hw4n4v"}}, nil).Once()
switchToLastUsedWorkspace = func(c *config.Context, workspaces []houston.Workspace) bool {
return false
}
Expand All @@ -284,21 +315,41 @@ func TestLoginSuccess(t *testing.T) {
t.Errorf("Login() error = %v, wantErr %v", err, false)
return
}
if gotOut := out.String(); !testUtil.StringContains([]string{"localhost"}, gotOut) {
t.Errorf("Login() = %v, want %v", gotOut, []string{"localhost"})
if gotOut := out.String(); !testUtil.StringContains([]string{"localhost", "ck05r3bor07h40d02y2hw4n4v"}, gotOut) {
t.Errorf("Login() = %v, want %v", gotOut, []string{"localhost", "ck05r3bor07h40d02y2hw4n4v"})
}

houstonMock.On("ListWorkspaces").Return([]houston.Workspace{{ID: "ck05r3bor07h40d02y2hw4n4v"}, {ID: "test-workspace-id"}}, nil).Once()
houstonMock.On("PaginatedListWorkspaces", 100, 0).Return([]houston.Workspace{{ID: "ck05r3bor07h40d02y2hw4n4v"}, {ID: "test-workspace-id"}}, nil).Once()
houstonMock.On("GetWorkspace", "ck05r3bor07h40d02y2hw4n4v").Return(&houston.Workspace{}, nil).Once()
houstonMock.AssertExpectations(t)
})

out = &bytes.Buffer{}
t.Run("when interactive set to true, prompt user to select workspace with pagination", func(t *testing.T) {
fs := afero.NewMemMapFs()
configYaml := testUtil.NewTestConfig("localhost")
afero.WriteFile(fs, config.HomeConfigFile, configYaml, 0o777)
config.InitConfig(fs)
config.CFG.Interactive.SetHomeString("true")
config.CFG.PageSize.SetHomeString("100")

defer testUtil.MockUserInput(t, "1")()

houstonMock := new(houstonMocks.ClientInterface)
houstonMock.On("GetAuthConfig", mock.Anything).Return(&houston.AuthConfig{LocalEnabled: true}, nil)
houstonMock.On("AuthenticateWithBasicAuth", mock.Anything, mock.Anything, mock.Anything).Return(mockToken, nil)
houstonMock.On("PaginatedListWorkspaces", 2, 0).Return([]houston.Workspace{{ID: "test-workspace-id"}, {ID: "test-workspace"}}, nil).Once()
switchToLastUsedWorkspace = func(c *config.Context, workspaces []houston.Workspace) bool {
return false
}

houstonMock.On("PaginatedListWorkspaces", 100, 0).Return([]houston.Workspace{{ID: "test-workspace-1"}, {ID: "test-workspace-2"}}, nil).Once()
houstonMock.On("GetWorkspace", "test-workspace-1").Return(&houston.Workspace{}, nil).Once()

out := &bytes.Buffer{}
if err := Login("localhost", false, "test", "test", houstonMock, out); (err != nil) != false {
t.Errorf("Login() error = %v, wantErr %v", err, false)
return
}
if gotOut := out.String(); !testUtil.StringContains([]string{"localhost", "test-workspace-id"}, gotOut) {
t.Errorf("Login() = %v, want %v", gotOut, []string{"localhost", "test-workspace-id"})
if gotOut := out.String(); !testUtil.StringContains([]string{"localhost", "test-workspace-1"}, gotOut) {
t.Errorf("Login() = %v, want %v", gotOut, []string{"localhost", "ck05r3bor07h40d02y2hw4n4v"})
}

houstonMock.AssertExpectations(t)
Expand Down