Default droplet options #61

petems · 2013-08-29T10:39:50Z

Fixes #56

Currently the tests around creating a default droplet are failing, possibly because the changes to what's seen as a default value for a new droplet.

@pearkes @blom mind having a look?

Not sure why, will look at it later

pearkes · 2013-09-01T21:34:44Z

Hey @petems! Sorry for the delay on this.

I got your specs over the hump in 823745e, but they are still failing. This is because the output text hasn't been changed.

I think in this case, we should omit the messages about defaults. I think that's expected and we don't need to reminds people.

Other than that, we also need to add the defaults into the configuration file, likely just what we had in the cli.rb default arguments.

Make sense? Nice work!

pearkes · 2013-09-01T21:45:00Z

Also – totally nitpicky but try to be descriptive in all of your commit messages – helps a lot when reviewing!

petems · 2013-09-01T23:27:05Z

Cool beans 👍

Yeah, sorry about that, I've got a bad habit of just saying "spiking" in commit messages when I'm stuck in a rut, I'll see if I can do a rebase of this later with better commit messages.

Thanks for the help 😸

No longer takes defaults from cli, uses ~/.tugboat file.

Now adds image, size and region

Checks defaults are created when no input given, and checks content of the created file

Allowing failures in 1.8.7 on travis now. It's EOL anyways 😉

petems · 2013-09-02T13:27:05Z

Ok, ended up making a lot of changes, and added some tests around the creation of the ~/.tugboat file, as I accidentally deleted one of the defaults (556b7b0), and I didn't notice until I looked at the diff, so I added tests for that case. Also, did a bit of rebasing and made the commit messages a bit more descriptive 👍

pearkes · 2013-09-03T15:50:35Z

.travis.yml

@@ -3,3 +3,6 @@ rvm:
  - 1.8.7
  - 1.9.3
  - 2.0.0
+matrix:
+  allow_failures:
+    - rvm: 1.8.7


Any way to get around this? If not, I'm ok with removing it.

Yeah, it's to do with the way rspec handles should_include, it's not an actual test failure. But since 1.8.7 is EOL I went the easy route heh.

I think if I switch the order up on the text I might be able to fix this, so I'll give it a shot 👍

pearkes · 2013-09-03T15:57:52Z

A couple more thoughts inline.

Also, right now, this will crash if the user doesn't have their defaults configured (if they just update the gem and don't re-run authorize). It'd be good to make it backwards compatible by letting the configuration default inside of the `configuration.rb, so if it doesn't get the config it falls back to the constants.

/Users/pearkes/code/pearkes/tugboat/lib/tugboat/config.rb:51:in `default_region': undefined method `[]' for nil:NilClass (NoMethodError)
    from /Users/pearkes/code/pearkes/tugboat/lib/tugboat/middleware/create_droplet.rb:10:in `call'
    from /Users/pearkes/code/pearkes/tugboat/lib/tugboat/middleware/inject_client.rb:33:in `call'
    from /Users/pearkes/code/pearkes/tugboat/lib/tugboat/middleware/check_configuration.rb:13:in `call'
    from /Users/pearkes/code/pearkes/tugboat/lib/tugboat/middleware/inject_configuration.rb:10:in `call'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/middleware-0.1.0/lib/middleware/runner.rb:31:in `call'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/middleware-0.1.0/lib/middleware/builder.rb:102:in `call'
    from /Users/pearkes/code/pearkes/tugboat/lib/tugboat/cli.rb:115:in `create'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/thor-0.18.1/lib/thor/command.rb:27:in `run'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/thor-0.18.1/lib/thor/invocation.rb:120:in `invoke_command'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/thor-0.18.1/lib/thor.rb:363:in `dispatch'
    from /Users/pearkes/.rvm/gems/ruby-2.0.0-p247/gems/thor-0.18.1/lib/thor/base.rb:439:in `start'
    from bin/tugboat:10:in `<main>'

pearkes · 2013-09-03T15:59:24Z

Oh! And last but not least, can we do default ssh_key_ids as well? Should be pretty straight forward.

Stops nil class errors if you don't have defaults in ~/.tugboat file Having difficulty with tests, but almost got it...

Not sure how to stub the config class to use backwards compatible ~/.tugboat file..

Tests now all green, will also grandfather config files from 0.0.8 >

petems · 2013-09-04T00:05:21Z

Whew, that was a long hack...

Ok, now uses ssh_key_ids, is backwards compatible with ~/.tugboat config files without defaults set, and I've added tests for that edge case (not super sure if it's the right way of doing it?)

@pearkes any thoughts? Still need to look into the 1.8.7 fix, I don't think it should be too hard to fix but didn't have time

…/tugboat into pm_default_region_option

petems · 2013-09-04T09:08:00Z

lib/tugboat/config.rb

    def ssh_port
      @data['ssh']['ssh_port']
    end

+    def default_region


I feel like there's a better way of doing this, rather than a ternary operator for each one, any suggestions welcome 👍

Lets us know for sure that defaults are being set by code...

petems · 2013-09-04T10:52:54Z

spec/middleware/ssh_droplet_spec.rb

@@ -4,7 +4,7 @@
  include_context "spec"

  before do
-    Kernel.stub!(:exec)


TIL http://blog.firsthand.ca/2010/09/rspec-stub-vs-stub.html, so I changed this 👍

pearkes · 2013-09-04T11:58:10Z

Yay! I added a little note about this in eca3fb5, otherwise looks awesome to me. Merging!

Default droplet options

petems added 3 commits August 8, 2013 12:03

Basic ask for region question...

d9ed883

First spike at default region, authorize spec failing...

526c6ae

Not sure why, will look at it later

Merge branch 'master' into pm_default_region_option

c09c22f

petems and others added 15 commits September 2, 2013 10:46

First run at adding default image and size to config

876a92d

Big refactor of how droplets are made

eb1f75c

No longer takes defaults from cli, uses ~/.tugboat file.

Fixed authorize spec

a254fd0

Change test environment ~/.tugboat setup

8fb3bdc

Now adds image, size and region

spec/create: stub requests based on spec defaults

853fcb0

Fix wrong wording for questions for authorize task

b9dc83e

Indentation

90a70bc

Config now writes defaults if none given

e4ca915

Now only uses config defaults if none given in cli

d137444

Specs now passing, changed to use config defaults

603404c

Fixed accidentally removed ssh_port parameter

556b7b0

Change defaults to what they were previously

25f4670

Add tests for the creation of ~/.tugboat file

c4f6c96

Checks defaults are created when no input given, and checks content of the created file

Size 33 no longer exists, now size 66

4cb7d30

Rspec should_include returns string in 1.8.7

55b6241

Allowing failures in 1.8.7 on travis now. It's EOL anyways 😉

pearkes reviewed Sep 3, 2013
View reviewed changes

petems added 4 commits September 3, 2013 17:47

Changed to constants for defaults

91a5b6d

Change to tenary operators for defaults

2baf03b

Now adds default ssh key to ~/.tugboat file

370cf6a

Fixed indentation

51e8582

petems added 6 commits September 3, 2013 23:48

Basic spec to make defaults backwards compatible

d7f43ea

Stops nil class errors if you don't have defaults in ~/.tugboat file Having difficulty with tests, but almost got it...

Rectify spec naming issue

f3e69a4

Wrong way round, do'h!

5929504

Commented out possible tests for defaults

42fe165

Not sure how to stub the config class to use backwards compatible ~/.tugboat file..

Fixed deprecation message

c79e9e8

Finally figured out how to mock instance variables...

52d50d6

Tests now all green, will also grandfather config files from 0.0.8 >

Merge branch 'pm_default_region_option' of https://github.com/pearkes…

b811239

…/tugboat into pm_default_region_option

petems reviewed Sep 4, 2013
View reviewed changes

petems added 3 commits September 4, 2013 10:08

Removing accidentally commited method

e2cde2e

Make SSH key default to empty if none given

a091acb

Changing environment setup to not use defaults

3d993d8

Lets us know for sure that defaults are being set by code...

petems reviewed Sep 4, 2013
View reviewed changes

authorize: add a note about the defaults

eca3fb5

specs: fix for new ID instructions

e59b1cc

pearkes added a commit that referenced this pull request Sep 4, 2013

Merge pull request #61 from pearkes/pm_default_region_option

d9c52d1

Default droplet options

pearkes merged commit d9c52d1 into master Sep 4, 2013

pearkes deleted the pm_default_region_option branch September 4, 2013 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default droplet options #61

Default droplet options #61

petems commented Aug 29, 2013

pearkes commented Sep 1, 2013

pearkes commented Sep 1, 2013

petems commented Sep 1, 2013

petems commented Sep 2, 2013

pearkes Sep 3, 2013

petems Sep 3, 2013

pearkes commented Sep 3, 2013

pearkes commented Sep 3, 2013

petems commented Sep 4, 2013

petems Sep 4, 2013

petems Sep 4, 2013

pearkes commented Sep 4, 2013

Default droplet options #61

Default droplet options #61

Conversation

petems commented Aug 29, 2013

pearkes commented Sep 1, 2013

pearkes commented Sep 1, 2013

petems commented Sep 1, 2013

petems commented Sep 2, 2013

pearkes Sep 3, 2013

Choose a reason for hiding this comment

petems Sep 3, 2013

Choose a reason for hiding this comment

pearkes commented Sep 3, 2013

pearkes commented Sep 3, 2013

petems commented Sep 4, 2013

petems Sep 4, 2013

Choose a reason for hiding this comment

petems Sep 4, 2013

Choose a reason for hiding this comment

pearkes commented Sep 4, 2013