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 all 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
6 changes: 3 additions & 3 deletions cmd/software/workspace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func TestWorkspaceSwitch(t *testing.T) {
houstonClient = houstonMock
defer func() { houstonClient = currentClient }()

houstonMock.On("GetWorkspace", wsID).Return(&houston.Workspace{}, nil).Once()
houstonMock.On("ValidateWorkspaceID", wsID).Return(&houston.Workspace{}, nil).Once()

err := workspaceSwitch(&cobra.Command{}, buf, []string{wsID})
assert.NoError(t, err)
Expand All @@ -152,7 +152,7 @@ func TestWorkspaceSwitch(t *testing.T) {
houstonClient = houstonMock
defer func() { houstonClient = currentClient }()

houstonMock.On("GetWorkspace", wsID).Return(&houston.Workspace{}, nil).Once()
houstonMock.On("ValidateWorkspaceID", wsID).Return(&houston.Workspace{}, nil).Once()

err := workspaceSwitch(&cobra.Command{}, buf, []string{wsID})
assert.NoError(t, err)
Expand All @@ -172,7 +172,7 @@ func TestWorkspaceSwitch(t *testing.T) {
houstonClient = houstonMock
defer func() { houstonClient = currentClient }()

houstonMock.On("GetWorkspace", wsID).Return(&houston.Workspace{}, nil).Once()
houstonMock.On("ValidateWorkspaceID", wsID).Return(&houston.Workspace{}, nil).Once()

err := workspaceSwitch(&cobra.Command{}, buf, []string{wsID})
assert.NoError(t, err)
Expand Down
1 change: 1 addition & 0 deletions houston/houston.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ type ClientInterface interface {
PaginatedListWorkspaces(pageSize int, pageNumber int) ([]Workspace, error)
DeleteWorkspace(workspaceID string) (*Workspace, error)
GetWorkspace(workspaceID string) (*Workspace, error)
ValidateWorkspaceID(workspaceID string) (*Workspace, error)
UpdateWorkspace(workspaceID string, args map[string]string) (*Workspace, error)
// workspace users and roles
AddWorkspaceUser(workspaceID, email, role string) (*Workspace, error)
Expand Down
23 changes: 23 additions & 0 deletions houston/mocks/ClientInterface.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 16 additions & 0 deletions houston/queries.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,22 @@ mutation UpdateDeployment($deploymentId: Uuid!, $payload: JSON!, $executor: Exec
}
`

ValidateWorkspaceIDGetRequest = `
query GetWorkspace(
$workspaceUuid: Uuid!
){
workspace(
workspaceUuid: $workspaceUuid
){
id
label
description
createdAt
updatedAt
}
}
`

WorkspacesGetRequest = `
query GetWorkspaces {
workspaces {
Expand Down
20 changes: 20 additions & 0 deletions houston/workspace.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,26 @@ func (h ClientImplementation) GetWorkspace(workspaceID string) (*Workspace, erro
return workspace, nil
}

// ValidateWorkspaceID - get a workspace
func (h ClientImplementation) ValidateWorkspaceID(workspaceID string) (*Workspace, error) {
req := Request{
Query: ValidateWorkspaceIDGetRequest,
Variables: map[string]interface{}{"workspaceUuid": workspaceID},
}

res, err := req.DoWithClient(h.client)
if err != nil {
return nil, handleAPIErr(err)
}

workspace := res.Data.GetWorkspace
if workspace == nil {
return nil, ErrWorkspaceNotFound{workspaceID: workspaceID}
}

return workspace, nil
}

// UpdateWorkspace - update a workspace
func (h ClientImplementation) UpdateWorkspace(workspaceID string, args map[string]string) (*Workspace, error) {
req := Request{
Expand Down
47 changes: 47 additions & 0 deletions houston/workspace_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,53 @@ func TestGetWorkspace(t *testing.T) {
})
}

func TestValidateWorkspaceID(t *testing.T) {
testUtil.InitTestConfig("software")

mockResponse := &Response{
Data: ResponseData{
GetWorkspace: &Workspace{
ID: "workspace-id",
Label: "label",
Description: "test description",
CreatedAt: "2020-06-25T22:10:42.385Z",
UpdatedAt: "2020-06-25T22:10:42.385Z",
},
},
}
jsonResponse, err := json.Marshal(mockResponse)
assert.NoError(t, err)

t.Run("success", func(t *testing.T) {
client := testUtil.NewTestClient(func(req *http.Request) *http.Response {
return &http.Response{
StatusCode: 200,
Body: io.NopCloser(bytes.NewBuffer(jsonResponse)),
Header: make(http.Header),
}
})
api := NewClient(client)

response, err := api.ValidateWorkspaceID("workspace-id")
assert.NoError(t, err)
assert.Equal(t, response, mockResponse.Data.GetWorkspace)
})

t.Run("error", func(t *testing.T) {
client := testUtil.NewTestClient(func(req *http.Request) *http.Response {
return &http.Response{
StatusCode: 500,
Body: io.NopCloser(bytes.NewBufferString("Internal Server Error")),
Header: make(http.Header),
}
})
api := NewClient(client)

_, err := api.ValidateWorkspaceID("workspace-id")
assert.Contains(t, err.Error(), "Internal Server Error")
})
}

func TestUpdateWorkspace(t *testing.T) {
testUtil.InitTestConfig("software")

Expand Down
41 changes: 30 additions & 11 deletions software/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,20 +54,24 @@ func basicAuth(username, password string, ctx *config.Context, client houston.Cl
return client.AuthenticateWithBasicAuth(username, password, ctx)
}

var switchToLastUsedWorkspace = func(c *config.Context, workspaces []houston.Workspace) bool {
var switchToLastUsedWorkspace = func(client houston.ClientInterface, c *config.Context) bool {
if c.LastUsedWorkspace == "" {
return false
}

for idx := range workspaces {
if c.LastUsedWorkspace == workspaces[idx].ID {
if err := c.SetContextKey("workspace", workspaces[idx].ID); err != nil {
return false
}
return true
}
// validate workspace
workspace, err := client.ValidateWorkspaceID(c.LastUsedWorkspace)
if err != nil || workspace != nil && workspace.ID != c.LastUsedWorkspace {
log.Debugf("last used workspace id is not valid: %s", err.Error())
return false
}
return false

if err := c.SetContextKey("workspace", workspace.ID); err != nil {
log.Debugf("unable to set workspace context: %s", err.Error())
return false
}

return true
}

// oAuth handles oAuth with houston api
Expand Down Expand Up @@ -127,6 +131,21 @@ 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 {
// To identify if the user has access to more than one workspace by setting workspace page size to 2, if so, take the user to the workspace switch flow.
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 +192,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 All @@ -194,7 +213,7 @@ func Login(domain string, oAuthOnly bool, username, password string, client hous

if len(workspaces) > 1 {
// try to switch to last used workspace in cluster
isSwitched := switchToLastUsedWorkspace(&c, workspaces)
isSwitched := switchToLastUsedWorkspace(client, &c)

if !isSwitched {
// show switch menu with available workspace IDs
Expand Down
Loading