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.

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

fix: restore previous session on coder server --dev #1821

Merged
merged 3 commits into from
May 27, 2022

Conversation

f0ssel
Copy link
Contributor

@f0ssel f0ssel commented May 26, 2022

Fixes #1747

@f0ssel f0ssel requested a review from mafredri May 26, 2022 23:00
@f0ssel f0ssel requested a review from a team May 26, 2022 23:04
cli/server.go Outdated
}
client.SessionToken = token.SessionToken

oldURL, err := cfg.URL().Read()
if err != nil {
return nil, xerrors.Errorf("write local url: %w", err)
Copy link
Member

Choose a reason for hiding this comment

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

This would fail if an old session didn't exist, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

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.

Thanks for taking a look at this issue! I think Kyle's comment warrants a small change but other than that, LGTM!

cli/server.go Outdated
@@ -336,10 +336,11 @@ func server() *cobra.Command {
return xerrors.Errorf("generate random admin password for dev: %w", err)
}
}
err = createFirstUser(cmd, client, config, devUserEmail, devUserPassword)
cleanupSession, err := createFirstUser(logger, cmd, client, config, devUserEmail, devUserPassword)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Would restore/restorePreviousSession be more descriptive here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

cli/server.go Outdated
if err != nil {
logger.Error(cmd.Context(), "failed to read current session token", slog.Error(err))
return
}
Copy link
Member

Choose a reason for hiding this comment

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

Thought: Do we need to consider cases where the user has tried the logout flow or ran coder login against the dev server? If so we could consider renaming the files to {url,session}.bak, and if those files are present on exit, (or startup), restore them. Perhaps even dev tokens could be a different format from non-dev, that way we could always identify if a restore case is appropriate or not.

Being able to restore on startup would also cover cases where the server is killed instead of graciously exited.

I'm probably overthinking this though, this change alone should help alleviate the most common cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think as a first pass I'd rather not try to guess, because I could see devs spending quite a while trying to figure out what's wrong before they realize it's the session. Currently if it's different that we would expect we just choose "do nothing", and I'm down to add more logic here as we use it and run into cases.

@f0ssel f0ssel enabled auto-merge (squash) May 27, 2022 16:53
@f0ssel f0ssel merged commit 8a5277e into main May 27, 2022
@f0ssel f0ssel deleted the f0ssel/restore-dev-session branch May 27, 2022 17:02
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.

Bug: Running code server --dev overwrites credentials
3 participants