Add a pub command for managing uploaders for packages.

Add a pub command for managing uploaders for packages.

BUG=7363
Committed: https://code.google.com/p/dart/source/detail?r=16230

[nweiz, 2012-12-15 02:13:26]

I went ahead and added this because it seems likely that we'll be doing a bunch
of uploader munging next week.

[Bob Nystrom, 2012-12-17 18:41:58]

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:77: } else if (url.origin == server.origin) {
What is this check for?

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.dart
File utils/pub/command_uploader.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:22: final usage = "pub uploader [options]
{add/remove} email";
"email" -> "<email>"

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:27: parser.addOption('server', defaultsTo:
'https://pub.dartlang.org',
Instead of a literal URI here, how about we make that a constant in
hosted_source?

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:28: help: 'The package server to which to upload
this package');
Make this relevant to this command.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:36: Uri get server => new
Uri.fromString(commandOptions['server']);
Does this fail gracefully if the argument isn't a valid URL?

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:45: var command =
commandOptions.rest.removeAt(0);
I think sometime soon I want to add real command support to args. I've been
waiting until we had some solid use code that needed it, and I think pub is at
the point no where it can be used to prove out the API.

https://codereview.chromium.org/11569046/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/io.dart#newcode569
utils/pub/io.dart:569: /// Handles a successful JSON-formatted response from
pub.dartlang.org.
io.dart is becoming a bit of a dumping ground. How about moving these to a new
library? Maybe "http.dart" or "web.dart"?

https://codereview.chromium.org/11569046/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/oauth2.dart#newcode90
utils/pub/oauth2.dart:90: return withClient(cache, fn);
Do we need to guard against getting stuck in an infinite loop here?

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/oauth2_test.dart
File utils/tests/pub/oauth2_test.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:172: 'token sucks).'));
If we show the error before retrying, I think we should also print out that it's
about to retry automatically.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
File utils/tests/pub/pub_uploader_test.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:38: runPub(args: ['uploader', 'add'],
output: USAGE_STRING, exitCode: 64));
It would be good to show a specific error message in addition to usage here.
Like, "Missing email address of uploader to add." or something like that.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:40: test('when run with an invalid
command', () => runPub(
Ditto. 'Unknown uploader command "foo". Expected "add" or "remove".' Or
something along those lines.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:45: test('creates an uploader', () {
"creates" -> "adds"

Here and elsewhere.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:50: server.handle('POST',
'/packages/pkg/uploaders.json', (request, response) {
Validate that the request includes the email?

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:57: 
Should we validate that pub prints out "Good job!" here (and elsewhere)?

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:63: test('deletes an uploader', () {
"deletes" -> "removes"

Here and elsewhere.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:69: (request, response) {
Validate that the request includes the email?

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:114: }));
We do this pretty frequently. Break out into a separate
respondTokenInvalid(response) function?

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:603: void runPub({List<String> args, Pattern
output, Pattern error,
List<String> -> List, here and elsewhere.

if it can contain non-strings. Shouldn't you be getting checked mode errors if
there are futures in there now?

[nweiz, 2012-12-17 21:18:48]

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_lish.dart
File utils/pub/command_lish.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_lish.dart#n...
utils/pub/command_lish.dart:77: } else if (url.origin == server.origin) {
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> What is this check for?

handleJsonError checks for an error in a format specific to certain responses
from pub.dartlang.org. This check ensures that we only expect that error format
from pub.dartlang.org, or localhost when testing etc.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.dart
File utils/pub/command_uploader.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:22: final usage = "pub uploader [options]
{add/remove} email";
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> "email" -> "<email>"

Done.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:27: parser.addOption('server', defaultsTo:
'https://pub.dartlang.org',
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Instead of a literal URI here, how about we make that a constant in
> hosted_source?

Right now using the hosted_source constant won't work, because we want to use
HTTPS for requests that include OAuth2 credentials but we don't want to use cURL
for just downloading packages. I'll add a TODO for that, though.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:28: help: 'The package server to which to upload
this package');
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Make this relevant to this command.

Done.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:36: Uri get server => new
Uri.fromString(commandOptions['server']);
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Does this fail gracefully if the argument isn't a valid URL?

I don't think Uri.fromString throws any parse errors - it'll do its best to
interpret any string. cURL will generate a reasonably sensible error, though.

https://codereview.chromium.org/11569046/diff/1/utils/pub/command_uploader.da...
utils/pub/command_uploader.dart:45: var command =
commandOptions.rest.removeAt(0);
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> I think sometime soon I want to add real command support to args. I've been
> waiting until we had some solid use code that needed it, and I think pub is at
> the point no where it can be used to prove out the API.

I thought about doing that here, but decided this would be easier for the time
being. It would be nice, though.

https://codereview.chromium.org/11569046/diff/1/utils/pub/io.dart
File utils/pub/io.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/io.dart#newcode569
utils/pub/io.dart:569: /// Handles a successful JSON-formatted response from
pub.dartlang.org.
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> io.dart is becoming a bit of a dumping ground. How about moving these to a new
> library? Maybe "http.dart" or "web.dart"?

Done.

https://codereview.chromium.org/11569046/diff/1/utils/pub/oauth2.dart
File utils/pub/oauth2.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/pub/oauth2.dart#newcode90
utils/pub/oauth2.dart:90: return withClient(cache, fn);
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Do we need to guard against getting stuck in an infinite loop here?

I don't think so -- even if the server keeps sending out-of-date credentials,
the user will have to perform an action each time, so it won't silently block
forever.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/oauth2_test.dart
File utils/tests/pub/oauth2_test.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/oauth2_test.d...
utils/tests/pub/oauth2_test.dart:172: 'token sucks).'));
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> If we show the error before retrying, I think we should also print out that
it's
> about to retry automatically.

The user has to re-authorize before retrying, which serves that purpose.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
File utils/tests/pub/pub_uploader_test.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:38: runPub(args: ['uploader', 'add'],
output: USAGE_STRING, exitCode: 64));
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> It would be good to show a specific error message in addition to usage here.
> Like, "Missing email address of uploader to add." or something like that.

We do, it's just on stderr. I didn't think it was worth checking for explicitly,
since then we have to change the test if we ever change the wording.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:40: test('when run with an invalid
command', () => runPub(
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Ditto. 'Unknown uploader command "foo". Expected "add" or "remove".' Or
> something along those lines.

See above.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:45: test('creates an uploader', () {
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> "creates" -> "adds"
> 
> Here and elsewhere.

Done.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:50: server.handle('POST',
'/packages/pkg/uploaders.json', (request, response) {
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Validate that the request includes the email?

Done.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:57: 
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Should we validate that pub prints out "Good job!" here (and elsewhere)?

Yeah, that seems reasonable. Done.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:63: test('deletes an uploader', () {
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> "deletes" -> "removes"
> 
> Here and elsewhere.

Done.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:69: (request, response) {
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> Validate that the request includes the email?

In this case it's embedded in the URL.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:114: }));
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> We do this pretty frequently. Break out into a separate
> respondTokenInvalid(response) function?

I think we only do it twice. And actually, this test shouldn't really exist;
it's part of the functionality of oauth2.dart, and there's already a test for it
there.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/test_pub.dart
File utils/tests/pub/test_pub.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/test_pub.dart...
utils/tests/pub/test_pub.dart:603: void runPub({List<String> args, Pattern
output, Pattern error,
On 2012/12/17 18:41:58, Bob Nystrom wrote:
> List<String> -> List, here and elsewhere.

Done.

> if it can contain non-strings. Shouldn't you be getting checked mode errors if
> there are futures in there now?

No, because I never actually instantiate a List<String> object.

[Bob Nystrom, 2012-12-17 21:26:58]

LGTM.

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
File utils/tests/pub/pub_uploader_test.dart (right):

https://codereview.chromium.org/11569046/diff/1/utils/tests/pub/pub_uploader_...
utils/tests/pub/pub_uploader_test.dart:69: (request, response) {
On 2012/12/17 21:18:48, nweiz wrote:
> On 2012/12/17 18:41:58, Bob Nystrom wrote:
> > Validate that the request includes the email?
> 
> In this case it's embedded in the URL.

Oh, sorry didn't notice. Does it URL-encode the email address then? It might be
nice to use something more like a real email address just to test that we do
that correctly.

[Sean Eagan, 2012-12-18 16:27:29]

DBC

I think "uploader" suggests there is an "upload" command, why not reuse
"publish" and do "publisher", i.e.:

pub publisher add jon@example.com
pub lisher add jon@example.com

or more terse would be separate commands for adding and removing:

pub grant jon@example.com
pub revoke jon@example.com

On 2012/12/15 02:13:26, nweiz wrote:
> I went ahead and added this because it seems likely that we'll be doing a
bunch
> of uploader munging next week.

[nweiz, 2012-12-18 18:54:42]

On 2012/12/18 16:27:29, seaneagan1 wrote:
> DBC
> 
> I think "uploader" suggests there is an "upload" command, why not reuse
> "publish" and do "publisher", i.e.:
> 
> pub publisher add mailto:jon@example.com
> pub lisher add mailto:jon@example.com
> 
> or more terse would be separate commands for adding and removing:
> 
> pub grant mailto:jon@example.com
> pub revoke mailto:jon@example.com
> 
> On 2012/12/15 02:13:26, nweiz wrote:
> > I went ahead and added this because it seems likely that we'll be doing a
> bunch
> > of uploader munging next week.

I'm not too worried about being terse, but it does make sense to say "publisher"
rather than "uploader".

[Bob Nystrom, 2012-12-18 18:59:07]

On 2012/12/18 18:54:42, nweiz wrote:
> On 2012/12/18 16:27:29, seaneagan1 wrote:
> > DBC
> > 
> > I think "uploader" suggests there is an "upload" command, why not reuse
> > "publish" and do "publisher", i.e.:
> > 
> > pub publisher add mailto:jon@example.com
> > pub lisher add mailto:jon@example.com
> > 
> > or more terse would be separate commands for adding and removing:
> > 
> > pub grant mailto:jon@example.com
> > pub revoke mailto:jon@example.com
> > 
> > On 2012/12/15 02:13:26, nweiz wrote:
> > > I went ahead and added this because it seems likely that we'll be doing a
> > bunch
> > > of uploader munging next week.
> 
> I'm not too worried about being terse, but it does make sense to say
"publisher"
> rather than "uploader".

+1.