Skip to content

Improve CLI logout flow #1692

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

Jump to bottom
Merged
merged 6 commits into from
May 24, 2022
Merged

Improve CLI logout flow #1692

merged 6 commits into from
May 24, 2022

Conversation

AbhineetJain
Copy link
Contributor

Improves CLI logout flow based on feedback in #1609

Subtasks

  • Do not delete the entire directory (only url, session and organization files)
  • Fix short description typo
  • Handle absence of URL and session files as the user being logged out, and suggest login.
  • Add unit tests

Fixes #1686

@AbhineetJain AbhineetJain marked this pull request as ready for review May 23, 2022 21:29
@AbhineetJain AbhineetJain requested review from f0ssel, dwahler, a team and Emyrk May 23, 2022 21:30
@Emyrk
Copy link
Member

Emyrk commented May 23, 2022

Should we delete the files? Or just make them empty? I'm good with either, just asking

dwahler
dwahler reviewed May 23, 2022
View reviewed changes
@AbhineetJain
Copy link
Contributor Author

Should we delete the files? Or just make them empty? I'm good with either, just asking

I prefer deleting them. We are checking for their presence at other places and raising errors about the user being logged out.

@AbhineetJain
Copy link
Contributor Author

Updated the flow as per discussion on #1686

When URL or Session file is not present:

$ coder logout
You are not logged in. Try logging in using 'coder login <url>'.
> Successfully logged out.

We are no longer throwing an error.

deansheather
deansheather approved these changes May 24, 2022
View reviewed changes
mafredri
mafredri approved these changes May 24, 2022
View reviewed changes
Copy link
Member

@mafredri mafredri left a comment

Choose a reason for hiding this comment

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

I think this looks great. I think we definitely want the os.Remove change, but approving nonetheless.

@Emyrk
Copy link
Member

Emyrk commented May 24, 2022

Can we make an endpoint on the api to delete the session token on the server side too?

@AbhineetJain
Copy link
Contributor Author

Can we make an endpoint on the api to delete the session token on the server side too?

@Emyrk Do you think we could do this in a new issue?

@AbhineetJain AbhineetJain force-pushed the abhineetjain/improve-logout branch from a4dc322 to 5f01dfd Compare May 24, 2022 15:54
@AbhineetJain
Copy link
Contributor Author

Updated the flow as per discussion on #1686

When URL or Session file is not present:

$ coder logout
You are not logged in. Try logging in using 'coder login <url>'.
> Successfully logged out.

We are no longer throwing an error.

This is updated now that we are only showing one message:

$ coder logout
> Are you sure you want to logout? (yes/no)
You are not logged in. Try logging in using 'coder login <url>'.

@Emyrk
Copy link
Member

Emyrk commented May 24, 2022

Can we make an endpoint on the api to delete the session token on the server side too?

@Emyrk Do you think we could do this in a new issue?

Yes that is fine

@AbhineetJain AbhineetJain requested a review from deansheather May 24, 2022 16:12
@AbhineetJain AbhineetJain mentioned this pull request May 24, 2022
Closed
@AbhineetJain
Copy link
Contributor Author

Can we make an endpoint on the api to delete the session token on the server side too?

@Emyrk Do you think we could do this in a new issue?

Yes that is fine

Filed this here: #1714

@AbhineetJain AbhineetJain merged commit 7ba6449 into main May 24, 2022
@AbhineetJain AbhineetJain deleted the abhineetjain/improve-logout branch May 24, 2022 17:11
kylecarbs pushed a commit that referenced this pull request Jun 10, 2022
@AbhineetJain @kylecarbs
Improve CLI logout flow (#1692)
41ba966 
* Improve CLI logout flow

* Fix lint error

* Make notLoggedInMessage a const

* successful logout with a msg when cfg files are absent

* use require, os.remove, show only one message, add prompt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mafredri mafredri mafredri approved these changes

@dwahler dwahler dwahler left review comments

@deansheather deansheather deansheather approved these changes

@f0ssel f0ssel Awaiting requested review from f0ssel

@Emyrk Emyrk Awaiting requested review from Emyrk

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

bug: do not delete the entire directory during logout
5 participants
@AbhineetJain @Emyrk @mafredri @dwahler @deansheather