Skip to content

Workspace: added workspace commands#29

Merged
98sean98 merged 13 commits intomainfrom
workspace-management
Jul 12, 2022
Merged

Workspace: added workspace commands#29
98sean98 merged 13 commits intomainfrom
workspace-management

Conversation

@Richard-B18
Copy link
Contributor

@Richard-B18 Richard-B18 commented Jul 8, 2022

next step is refactoring the other commands that used the old workspace prompting

@Richard-B18 Richard-B18 requested a review from 98sean98 July 8, 2022 05:39
@Richard-B18 Richard-B18 marked this pull request as draft July 8, 2022 08:38
@Richard-B18 Richard-B18 self-assigned this Jul 8, 2022
@Richard-B18 Richard-B18 marked this pull request as ready for review July 8, 2022 08:48
@Richard-B18 Richard-B18 linked an issue Jul 8, 2022 that may be closed by this pull request
Comment on lines +23 to +25
if not "username" in context.global_config["WORKSPACE"]:
click.secho("Error in getting workspace")
raise click.Abort()
Copy link
Contributor

Choose a reason for hiding this comment

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

The error statement should be more precise
The fact that we could not get workspace means that the user has not done login through cli right?
So maybe something like please login would be a better error statement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uh aren't you dealing with this issue now? but yeah I agree, I was just trying to finish up everything. But on top of that, yeah I think it makes sense to ask the user to login, but maybe also make sure they choose the right workspace to create a cloud profile in

Copy link
Contributor

Choose a reason for hiding this comment

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

There should be an is_authenticated decorator to check for auth state already. But it's kinda useful to add this additional check for whether there's a workspace username in the global config as well; not entirely necessary because this conditional would only fail if the user manually edits the global config file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just wrote that so i could reference it later

@Richard-B18 Richard-B18 marked this pull request as draft July 11, 2022 07:29
Comment on lines +10 to +16
@click.command()
@pass_deploifai_context_obj
@is_authenticated
def list(context: DeploifaiContextObj):
"""
Lists out all available workspaces
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

list as a function name might cause errors in python
please suggest an alternate name

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. But I think we still want it to be workspace list command. So whatever function name we give it, we can just override with a command name argument put in the click.command decorator call.

@98sean98 98sean98 force-pushed the workspace-management branch from 68778da to d1401e2 Compare July 12, 2022 07:07
Cloud Profile Create - Tested, working as intended
Project Create - Tested, working as intended
Workspace set and list -  Tested, working as intended
Create cloud profile and get projects function -  Changed workspace to be string instead of list
@kschauhan3 kschauhan3 marked this pull request as ready for review July 12, 2022 08:34
Copy link
Contributor

@98sean98 98sean98 left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +23 to +25
if not "username" in context.global_config["WORKSPACE"]:
click.secho("Error in getting workspace")
raise click.Abort()
Copy link
Contributor

Choose a reason for hiding this comment

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

Refactor into a decorator function.

Comment on lines +28 to +31
for w in workspaces_from_api:
if w["username"] == workspace:
command_workspace = w
break
Copy link
Contributor

Choose a reason for hiding this comment

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

quite unnecessary because the if(any... statement would have returned true if the workspace name provided by the user is in the list

Comment on lines +23 to +25
if not "username" in context.global_config["WORKSPACE"]:
click.secho("Error in getting workspace")
raise click.Abort()
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be an is_authenticated decorator to check for auth state already. But it's kinda useful to add this additional check for whether there's a workspace username in the global config as well; not entirely necessary because this conditional would only fail if the user manually edits the global config file.

Comment on lines +10 to +16
@click.command()
@pass_deploifai_context_obj
@is_authenticated
def list(context: DeploifaiContextObj):
"""
Lists out all available workspaces
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. But I think we still want it to be workspace list command. So whatever function name we give it, we can just override with a command name argument put in the click.command decorator call.

@98sean98 98sean98 merged commit 4950a93 into main Jul 12, 2022
@98sean98 98sean98 deleted the workspace-management branch July 12, 2022 09:32
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.

manage workspaces in global config

3 participants