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

feat(config): added initial keyring support (#83) #350

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MarinX
Copy link
Contributor

@MarinX MarinX commented Dec 30, 2024

This PR adds initial support for the keyring.
I used zalando/go-keyring because of recent commits on repo (compared to 99designs/keyring).
When I say initial, I mean it's open for discussion and adding more features (create auth command and save it to keyring?).

How it works:
If a username is set and the password is empty, check the keyring for service upctl where the username is coming either from the config file or env.
This type of logic does not break current.

Problem:
This PR does not read the username from the keyring, so it still needs a username from config or env. Maybe to introduce a config command --keyring ?

For me, it works by using UPCLOUD_USERNAME and creating username/password upctl services on keyring so I can switch easily between accounts without providing passwords.

@MarinX MarinX requested a review from a team as a code owner December 30, 2024 20:58
@kangasta
Copy link
Contributor

Thanks for the pull request! We'll do some testing with this and let's discuss what would be the best approach to get this merged.

We are also working on adding tokens support for authentication (product radar), so probably best to start with minimal implementation, like you described above, and add more features once the initial functionality is on the main.

internal/config/config.go Outdated Show resolved Hide resolved
Copy link
Contributor

@kangasta kangasta left a comment

Choose a reason for hiding this comment

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

Did some testing with this and seems to work nicely on mac 👍 We'll have to still confirm that this does not have conflicts with the upcoming API tokens feature.

Added one comment about the unittest and seems that at least the ubuntu test workflow needs a step for installing keyring support. I guess something like the snippet below would work for that 🤔

- name: Install keyring test dependencies on Ubuntu
  run: apt install gnome-keyring
  if: matrix.os == 'ubuntu-latest'

_, err = tmpFile.WriteString("username: sdkfo")
assert.NoError(t, err)

err = keyring.Set("upctl", "sdkfo", "foo")
Copy link
Contributor

Choose a reason for hiding this comment

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

This will end up in the systems default keyring, so could use more descriptive username, e.g unittest, and a cleanup function for removing this with t.Cleanup.

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.

3 participants