-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add logout sub command #1737
base: main
Are you sure you want to change the base?
Add logout sub command #1737
Conversation
Hi, Please dont hesitate to give feedback or any ideas of how to improve the PR, in case you have that! I would be happy to implement suggested changes, or elaborate further on the PR :) |
@RicNord Thank you for this contribution! We require a CLA to accept contributions. If you're open to signing one, could you send an email to [email protected], and I'll kickstart the process (this is a manual process still). The new command looks great. As for the scope, I think we shouldn't rewrite the databrickscfg file. I'd rather keep parity between login/logout and focus exclusively on OAuth. If login supported non-OAuth profiles then it would make sense. |
libs/auth/cache/file.go
Outdated
if err != nil { | ||
return fmt.Errorf("marshal: %w", err) | ||
} | ||
return os.WriteFile(c.fileLocation, raw, ownerReadWrite) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an unexported function to write the configuration to disk?
It can be shared between Store
and Delete
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in commit d037ec3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @pietern. I took a look at Azure CLI's implementation as a reference (https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/_profile.py#L308), where they do actually remove the metadata about those subscriptions that the user is logging out of. It's also convenient in that it means that users don't have to manually modify the .databrickscfg
file directly, which isn't very user-friendly. Even if login
only supports the U2M OAuth flow, it might be nice that logout
cleans up everything about the profile. I think the only real difference is that a user could login via a profile name, rather than needing to type the host (and account ID, if necessary).
Signed the CLA :) Please let me know how to proceed with the scope of the PR! |
If integration tests don't run automatically, an authorized user can run them manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Changes
logout
command #1639This PR includes a new sub cmd:
auth logout [PROFILE]
. That will "logout" a specified profile, similar toaz logout
.Logout process looks like this:
~/.databrickscfg
) and tokenCache (Default:~/.databricks/token-cache.json
)4. Overwrite profile section in config file, but without any sensitive values, PAT token etc.Tests