Skip to content
This repository has been archived by the owner on Dec 18, 2020. It is now read-only.

implemented a CLI for generating and deleting tokens #4

Merged
merged 11 commits into from
Jan 22, 2016
Merged

implemented a CLI for generating and deleting tokens #4

merged 11 commits into from
Jan 22, 2016

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Jan 20, 2016

There's now a bin for generating and deleting tokens:

screen shot 2016-01-20 at 2 54 51 pm

reviewers: @nexdrew

@nexdrew
Copy link
Contributor

nexdrew commented Jan 20, 2016

  1. [question] The bin doesn't match the package/repo name. Is that intended?
  2. [question] Is there any significance to the username and email? I mean, they're not really used for anything, and these tokens will typically be for CI servers, so anything goes for the time being, correct?
  3. [feature] The inquirer prompt is cool, but it might be nice to allow those options to be passed in on the command line as an alternative to the prompt, to better support automation. This can be added later though.
  4. [feature] Hopefully we don't end up with hundreds or thousands of tokens; otherwise listing them all becomes unwieldy. A command to "delete all" might be nice. Again, can be added later.

Otherwise, this is looking good.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 21, 2016

@nexdrew

[question] Is there any significance to the username and email?

Anything goes for these currently, I figured it would be nice for folks to be able to pick an email and username; this way if they have their CI server, as an example, publishing modules the info int the website can reference the CI server.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 21, 2016

[feature] The inquirer prompt is cool, but being able to pass an argument would be nice.

Given points 3. and 4. I think it's worth adding this command line flag.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 21, 2016

@nexdrew okay, I think this is ready for review again; I'm going to start playing with getting two auth strategies installed in my local npmo-docker-compose setup.

@nexdrew
Copy link
Contributor

nexdrew commented Jan 21, 2016

Would be nice to accept name and email as CLI options for the generate command, but it's not necessary right now.

I vote 🐑

@nexdrew
Copy link
Contributor

nexdrew commented Jan 21, 2016

Oh, bin name in the README examples needs to be updated

@nexdrew
Copy link
Contributor

nexdrew commented Jan 22, 2016

Will the use of hashes in Redis affect other auth strategies? I guess if they attempt to grab a key of the incorrect type, they'll just error out and npm-auth-ws will just move on to the next plugin?

This will be fine as long as other strategies are not attempting to iterate over all keys without expecting different types.

@nexdrew
Copy link
Contributor

nexdrew commented Jan 22, 2016

Might be nice to include a timestamp in the Redis value for when the token was created.

@nexdrew
Copy link
Contributor

nexdrew commented Jan 22, 2016

Might also be nice to sort the tokens before displaying the list, but at least the tokens are ordered consistently.

@nexdrew
Copy link
Contributor

nexdrew commented Jan 22, 2016

Sorry, I can think of several things that "might be nice" but are completely unnecessary. Feel free to ignore those comments.

@bcoe
Copy link
Contributor Author

bcoe commented Jan 22, 2016

@nexdrew

Should these tests be cleaning up after themselves by deleting the tokens they generate?

I think they probably should be, I was being lazy; I'll patch this.

Might be nice to include a timestamp in the Redis value for when the token was created.

Might as well? I'll add this.

Will the use of hashes in Redis affect other auth strategies?

We're already mixing and matching the two in the wild (our earliest strategies use serialized JSON, our newer strategies have moved to hashes). I think we're cool.

bcoe added a commit that referenced this pull request Jan 22, 2016
implemented a CLI for generating and deleting tokens
@bcoe bcoe merged commit 9eee399 into master Jan 22, 2016
@bcoe bcoe deleted the cli branch January 22, 2016 23:52
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.

None yet

2 participants