Skip to content
This repository has been archived by the owner on Oct 4, 2024. It is now read-only.

Move default location for credentials to .near-credentials #339

Merged
merged 10 commits into from
Apr 30, 2020

Conversation

janedegtiareva
Copy link
Contributor

@janedegtiareva janedegtiareva commented Apr 29, 2020

Also fix an issue that login is not honoring the system-wide keystore
and creating its own version.

The new location for keys is, for example on windows with default env: C:\Users\userid.near-credentials\default

Also fix an issue that login is not honoring the system-wide keystore
and creating its own version.
@janedegtiareva
Copy link
Contributor Author

janedegtiareva commented Apr 29, 2020

Tested (on windows)

  • near login
  • make sure the key is stored in the appropriate location
  • near send

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

This is a great idea! I was just thinking about something like this yesterday. But I thought the implementation might look a little different and I want to hold this up with a conversation, if I can.

A couple ideas for changing this:

  1. Why limit it to only storing credentials in the home directory? What if I want to be logged in with certain credentials most of the time, but override for certain projects/folders? We could make this look for .near-credentials in the current directory, and if none found, the next directory up, and so forth up to ~ (or maybe / but we'd probably run into permissions issues), and finally, at the end of the chain, the old ./neardev location.
  2. Do we only want to store credentials in here? Could we make this a .nearconfig and make it use a toml format (or toml-like, as with .gitconfig)?

Copy link
Contributor

@mikedotexe mikedotexe left a comment

Choose a reason for hiding this comment

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

Wow, I'm surprised how little code this took when you use MergeKeyStore! Awesome 🙌

@mikedotexe
Copy link
Contributor

This is a great idea! I was just thinking about something like this yesterday. But I thought the implementation might look a little different and I want to hold this up with a conversation, if I can.

A couple ideas for changing this:

  1. Why limit it to only storing credentials in the home directory? What if I want to be logged in with certain credentials most of the time, but override for certain projects/folders? We could make this look for .near-credentials in the current directory, and if none found, the next directory up, and so forth up to ~ (or maybe / but we'd probably run into permissions issues), and finally, at the end of the chain, the old ./neardev location.
  2. Do we only want to store credentials in here? Could we make this a .nearconfig and make it use a toml format (or toml-like, as with .gitconfig)?

I think this is good stop-gap before we approve and implement the NEP where the key management does have an order of lookups described:
https://github.com/nearprotocol/NEPs/blob/6b6e9ce2ab5f9cb36b69accae3970d8ea997a29b/text/0000-near-shell.md#key-management

We're at that odd spot where the NEP isn't "approved" but we could really use some of the functionality. I think this is a PR that will grow into the NEP. For instance, the NEP states that keys be in .near-credentials although we're keeping the project's .neardev for backwards compatibility. I think this smoothly leans into the future of shell, but will be improved as time goes on. And since the default is to save it to the home directory, this is a step in the right direction.

Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

Sounds good to me

@janedegtiareva
Copy link
Contributor Author

janedegtiareva commented Apr 29, 2020

This is a great idea! I was just thinking about something like this yesterday. But I thought the implementation might look a little different and I want to hold this up with a conversation, if I can.

A couple ideas for changing this:

  1. Why limit it to only storing credentials in the home directory? What if I want to be logged in with certain credentials most of the time, but override for certain projects/folders? We could make this look for .near-credentials in the current directory, and if none found, the next directory up, and so forth up to ~ (or maybe / but we'd probably run into permissions issues), and finally, at the end of the chain, the old ./neardev location.
  2. Do we only want to store credentials in here? Could we make this a .nearconfig and make it use a toml format (or toml-like, as with .gitconfig)?

These are all good options, the question is more of timing (i.e. specifically do we want to do all that before RL1 or not -- I thought we decided in a quick chat on Tuesday that we want to move to home dir right now). Maybe let's have this discussion in near/NEPs#31?

@vgrichina
Copy link
Contributor

Do we only want to store credentials in here? Could we make this a .nearconfig and make it use a toml format (or toml-like, as with .gitconfig)?

@chadoh I'm not convinced we should use toml. Other than that I agree with @janedegtiareva that this is out of scope for this PR. Thanks for getting backwards compatibility for ./neardev, that's good move before event.

Copy link
Contributor

@vgrichina vgrichina left a comment

Choose a reason for hiding this comment

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

Looks good to me, ./neardev seems to work fine as well.

@mikedotexe
Copy link
Contributor

@janedegtiareva lint issues, but then ship it, Captain

@janedegtiareva janedegtiareva merged commit 3dd9128 into master Apr 30, 2020
@chadoh chadoh mentioned this pull request May 6, 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.

4 participants