[WIP] Adds machine token commands for 1.0 #1182

ronan · 2016-09-09T18:32:14Z

~~Requires: #1179~~

This PR:

Implements machine-token:list, machine-token:delete
Adds unit tests for above
Creates a new non-static Session object
Adds basis for DI of session object

Still todo:

Actual injection of session object using Robo container
Get behat tests working
~~Get output working (requires rebase onto robo-runner branch)~~

pantheon-mentionbot · 2016-09-09T18:32:17Z

@ronan, thanks for your PR! By analyzing the blame information on this pull request, we identified @TeslaDethray to be a potential reviewer

ronan · 2016-09-09T18:34:33Z

src/Commands/MachineToken/DeleteCommand.php

+        // Find the token. Will throw an exception if it doesn't exist.
+        $machine_token = $user->machine_tokens->get($machine_token_id);
+        if (empty($machine_token)) {
+            throw new \Exception(vsprintf('There are no machine tokens with the id %s.', [$machine_token_id]), 1);


@TeslaDethray: Is this the right way to handle command failures? Do we want to use more specific Exceptions (TerminusNotFoundException for example).

I'm not in love with gluing the id into the error message before adding it to the Exception for language translation and management reasons but this is fine for now I guess.

greg-1-anderson · 2016-09-09T20:48:16Z

src/Commands/MachineToken/ListCommand.php


+        // Return the output data.
+        return new RowsOfFields($data);


ronan · 2016-09-13T15:48:13Z

@TeslaDethray I think this is more or less ready to go except the Behat tests won't pass until the auth:login command is merged. And also until we can have a way of auto-loading the new FeatureContext.php class. Not sure how to do that.

Do we want to block this until we can get the Behat tests working? Or merge and iterate?

TeslaDethray · 2016-09-14T15:53:30Z

The new FeatureContext.php should be loaded whenever you run the default_10 suite. @ronan

ronan · 2016-09-14T17:08:26Z

@TeslaDethray Not in my experience. The global autoload seems to load up the old one:

https://github.com/pantheon-systems/terminus/blob/master/tests/config/behat.yml#L2

Not sure what I'm doing wrong here.

I did get it to work by making a behat-10.yml file and using that instead of behat.yml which seems much simpler. I still can't get my behat tests to authenticate though. I'll keep digging but I might be out of my depth a bit.

TeslaDethray · 2016-09-14T17:11:07Z

@ronan Would it be possible to split the wonderful work you've done with the Session here into a separate PR?

ronan · 2016-09-14T17:11:49Z

Yeah. that'll probably help unstick this. I'll have a new pr in a few mins

ronan · 2016-09-14T20:44:42Z

I got behat tests working but this requires:

And a rebase

greg-1-anderson · 2016-09-14T22:11:50Z

Those PRs all look mergable to me.

coveralls · 2016-09-15T19:50:10Z

Coverage remained the same at 14.038% when pulling a264d57 on AL-538-machine-tokens-list into 8dde1f0 on master.

coveralls · 2016-09-15T19:50:10Z

Coverage remained the same at 14.038% when pulling a264d57 on AL-538-machine-tokens-list into 8dde1f0 on master.

ronan · 2016-09-15T19:56:43Z

Ok so this is passing now. All it's waiting for is guidance on the preferred way to implement the y/n delete confirmation

@TeslaDethray ^

greg-1-anderson · 2016-09-15T20:30:59Z

There is an example of how to ask for missing arguments in #1206; however, interact isn't really the right answer for confirmation messages.

coveralls · 2016-09-16T16:41:19Z

Coverage remained the same at 14.038% when pulling 8d10b09 on AL-538-machine-tokens-list into 57a0568 on master.

ronan · 2016-09-16T18:24:42Z

src/Commands/MachineToken/DeleteCommand.php

+
+        $this->log()->notice('Deleting {token} ...', ['token' => $name]);
+        $response = $machine_token->delete();
+        if ($response['status_code'] == 200) {


I don't think we should be leaking the response status code out of the model here. I'd rather have delete() throw an exception (which the command could simply let fall through to the runner for simplicity). That change will probably break the old stuff though so I don't know if I should proceed or leave it as is and try and clean it up later.

By "break the old stuff" I mean "require a change to the old stuff" I woudn't leave the existing code in a broken state obviously

This status-code check must've persisted for some time because it's not necessary anymore. We can just remove it.

ronan · 2016-09-16T18:27:52Z

src/Commands/MachineToken/DeleteCommand.php


+        // Find the token. Will throw an exception if it doesn't exist.
+        $machine_token = $user->machine_tokens->get($machine_token_id);


Here I'm letting the exception thrown by get() fall through and get caught by the runner. This works fine (and saves a few lines of code) but the Robo runner isn't familiar with our translatable exceptions. That means the output when there is an error here is:

[error] Could not find {model} "{id}"

We have 3 options:

Catch and rethrow here with a regular, untranslatable exception

Override the runner to output a "translated" version of the exception error.

Change TerminusException to do the replacement itself so that getMessage() always returns a valid error message

I favor #3. We'll have to revisit it a bit when we actually start looking at translating error messages, but it should be backwards compatible with the old Terminus runner and forwards compatible with the new one (while not prematurely interpolating error strings).

@TeslaDethray ^^

I also favor approach approach 3.

coveralls · 2016-09-16T18:46:10Z

Coverage increased (+1.4%) to 15.428% when pulling 18f73eb on AL-538-machine-tokens-list into 3af361d on master.

coveralls · 2016-09-16T21:07:28Z

Coverage increased (+0.07%) to 16.098% when pulling 1df3b87 on AL-538-machine-tokens-list into e6e0e12 on master.

coveralls · 2016-09-16T21:12:19Z

Coverage increased (+0.07%) to 16.098% when pulling 1df3b87 on AL-538-machine-tokens-list into e6e0e12 on master.

coveralls · 2016-09-16T21:22:23Z

Coverage increased (+0.07%) to 16.098% when pulling 1df3b87 on AL-538-machine-tokens-list into e6e0e12 on master.

coveralls · 2016-09-16T22:18:48Z

Coverage increased (+0.07%) to 16.098% when pulling f93a411 on AL-538-machine-tokens-list into 9415d9b on master.

TeslaDethray · 2016-09-16T22:21:29Z

php/Terminus/Commands/MachineTokensCommand.php

@@ -94,17 +95,18 @@ public function delete($args, $assoc_args) {
      'Deleting {name} ...',
      array('name' => $name)
    );
-    $response = $machine_token->delete();
-    if ($response['status_code'] == 200) {


How good of you to fix the original, too! 💐

TeslaDethray · 2016-09-16T22:25:03Z

php/Terminus/Models/MachineToken.php

@@ -17,15 +19,17 @@ public function __construct($attributes, array $options = []) {

  /**
   * Deletes machine token
-   *
-   * @return array
+   * @return null


This should be @return void.

Stack Exchange / PHPDoc wiki page says to omit @return if the return type is void.

http://stackoverflow.com/questions/2061550/phpdoc-return-void-necessary

I have no objection if we knowingly wish to diverge from the standard, though.

It doesn't pass the linter without a @return tag so I'll add @return void for now.

@greg-1-anderson This repo is already divergent.
@ronan Since PHP always returns null in the absence of a return, the difference between @return void and @return null is that void denotes to users that its return value will be of no use to them. null is used in conjunction with other possible return types, e.g. @return null|string

@TeslaDethray I dig. Change made.

coveralls · 2016-09-16T22:48:28Z

Coverage increased (+0.07%) to 16.098% when pulling f76a18e on AL-538-machine-tokens-list into 9415d9b on master.

coveralls · 2016-09-16T22:48:28Z

Coverage increased (+0.07%) to 16.098% when pulling f76a18e on AL-538-machine-tokens-list into 9415d9b on master.

TeslaDethray · 2016-09-16T23:27:25Z

+1

ronan added the Changes Requested label Sep 9, 2016

ronan reviewed Sep 9, 2016
View reviewed changes

ronan force-pushed the AL-538-machine-tokens-list branch from 2e5d912 to 2fd9ece Compare September 9, 2016 20:14

greg-1-anderson reviewed Sep 9, 2016
View reviewed changes

src/Commands/MachineToken/ListCommand.php

// Return the output data.

return new RowsOfFields($data);

Copy link

Member

greg-1-anderson Sep 9, 2016

Choose a reason for hiding this comment

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

Yay!

ronan force-pushed the AL-538-machine-tokens-list branch 2 times, most recently from bfdcfe6 to cab83b1 Compare September 13, 2016 15:42

ronan force-pushed the AL-538-machine-tokens-list branch from cab83b1 to 34cd341 Compare September 14, 2016 18:59

ronan force-pushed the AL-538-machine-tokens-list branch from afee69c to 6305f0f Compare September 14, 2016 23:12

Adds machine token commands for 1.0

a264d57

ronan force-pushed the AL-538-machine-tokens-list branch from 37f7115 to a264d57 Compare September 15, 2016 19:35

Merge branch 'master' into AL-538-machine-tokens-list

8d10b09

TeslaDethray and others added 2 commits September 16, 2016 11:08

Merge branch 'master' into AL-538-machine-tokens-list

dcd8f6b

Remove confirmation dialog from mt:delete

18f73eb

ronan commented Sep 16, 2016

View reviewed changes

ronan added Needs Review Changes Requested and removed Changes Requested labels Sep 16, 2016

Ronan Dowling added 2 commits September 16, 2016 15:56

Updated exception handling in mt:delete

005e173

Merge branch 'master' into AL-538-machine-tokens-list

db59595

ronan removed the Changes Requested label Sep 16, 2016

Removed unneeded assignment

1df3b87

Merge branch 'master' into AL-538-machine-tokens-list

f93a411

TeslaDethray reviewed Sep 16, 2016

View reviewed changes

Ronan Dowling added 2 commits September 16, 2016 17:37

Added annotations to machine-token:list

609f3ed

Fixed return value

f76a18e

TeslaDethray added Ready to Ship and removed Needs Review labels Sep 16, 2016

ronan merged commit cc0a0d7 into master Sep 19, 2016

ronan deleted the AL-538-machine-tokens-list branch September 19, 2016 14:49


		// Find the token. Will throw an exception if it doesn't exist.
		$machine_token = $user->machine_tokens->get($machine_token_id);

[WIP] Adds machine token commands for 1.0 #1182

[WIP] Adds machine token commands for 1.0 #1182

Conversation

ronan commented Sep 9, 2016 • edited

pantheon-mentionbot commented Sep 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronan commented Sep 13, 2016

TeslaDethray commented Sep 14, 2016

ronan commented Sep 14, 2016

TeslaDethray commented Sep 14, 2016

ronan commented Sep 14, 2016

ronan commented Sep 14, 2016 • edited by greg-1-anderson

greg-1-anderson commented Sep 14, 2016

coveralls commented Sep 15, 2016 • edited

coveralls commented Sep 15, 2016

ronan commented Sep 15, 2016

greg-1-anderson commented Sep 15, 2016

coveralls commented Sep 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronan Sep 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 16, 2016 • edited

coveralls commented Sep 16, 2016 • edited

coveralls commented Sep 16, 2016 • edited

coveralls commented Sep 16, 2016 • edited

coveralls commented Sep 16, 2016 • edited

TeslaDethray Sep 16, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 16, 2016

coveralls commented Sep 16, 2016 • edited

TeslaDethray commented Sep 16, 2016

ronan commented Sep 9, 2016 •

edited

ronan commented Sep 14, 2016 •

edited by greg-1-anderson

coveralls commented Sep 15, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

ronan Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited

TeslaDethray Sep 16, 2016 •

edited

coveralls commented Sep 16, 2016 •

edited