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

command/logout: Add terraform logout command #24048

Merged
merged 3 commits into from
Feb 6, 2020

Conversation

alisdair
Copy link
Contributor

@alisdair alisdair commented Feb 6, 2020

Use terraform logout to remove stored credentials for a remote service host. This is the corresponding command to terraform login.

Screenshots

Normal successful usage:
success-credentials-file

For a specific hostname (e.g. a TFE instance):
success-hostname-override

Using a credentials helper:
success-credentials-helper

When attempting to remove credentials which were manually configured (e.g. in ~/.terraformrc):
error-manually-configured

If the credentials helper fails:
error-credentials-helper

When already logged out:
already-logged-out

@alisdair alisdair requested a review from a team February 6, 2020 19:16
@alisdair alisdair force-pushed the alisdair/terraform-logout branch from 7fe6e95 to 91ca9e5 Compare February 6, 2020 19:21
command/logout.go Outdated Show resolved Hide resolved
command/logout.go Outdated Show resolved Hide resolved
Use terraform logout to remove stored credentials for a remote service
host.
@alisdair alisdair force-pushed the alisdair/terraform-logout branch from 73761e5 to 325f8a8 Compare February 6, 2020 20:01
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

This looks great overall! I think I found a potential panic but I also may be misreading the code

}
}

if credsCtx.Location == cliconfig.CredentialsInOtherFile {
Copy link
Contributor

Choose a reason for hiding this comment

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

if credsCtx is nil, this will panic, and it sounds like that's not impossible .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot!

Unless I'm missing something, I think it actually was impossible. Despite the comments, the services were always initialized with a cliconfig.CredentialsSource, both in real code and tests. (This code was cargo culted from the login command, which is why I'm unsure.)

So in 7ff5878 I've removed the check (for both login and logout) and replaced it with a type assertion. I'm not 100% sure this is the right move, but tests pass and the commands work, and at least this way the panic will happen on a more useful line 🤷‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

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

Ha! That is an excellent step forward - As you said, if it happens out in the wild, we'll know when and where 😁

The type assertion checks on the credentials source are unnecessary, and
the alternative code path they allow would panic.
@alisdair alisdair merged commit afd792d into master Feb 6, 2020
@alisdair alisdair deleted the alisdair/terraform-logout branch February 6, 2020 20:59
@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants