Skip to content

Comments

[OHAI-518] remove *.static.cloud-ips.com#345

Merged
sersut merged 7 commits intochef:masterfrom
squaresurf:pr-215-fix
Oct 27, 2014
Merged

[OHAI-518] remove *.static.cloud-ips.com#345
sersut merged 7 commits intochef:masterfrom
squaresurf:pr-215-fix

Conversation

@squaresurf
Copy link

Fixes issues with Pull Request #215

  • fixes tests
  • moves require "resolve" to top of file

gtmanfred and others added 3 commits October 18, 2013 13:56
Rackspace no longer sets the reverse dns for cloud servers as the spin
up.

https://community.rackspace.com/general/f/34/t/623
Fixes issues with Pull Request #215
- fixes tests
- moves `require "resolve"` to top of file
@sersut
Copy link

sersut commented Jun 24, 2014

Thanks for the patch @squaresurf & @gtmanfred. I'm 👍 on this.

In order to get this patch in we need @squaresurf to sign our CLA:

https://github.com/opscode/chef/blob/master/CONTRIBUTING.md#contributor-license-agreement-cla

@opscode/client-eng anyone has some cycles to check this one out?

@squaresurf
Copy link
Author

I signed the CLA via the new supermarket site. If that isn't live, then would you post a link where I should sign the CLA?

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider locking this to Resolv::ResolvError and whatever the exception is for the timout.

@btm
Copy link
Contributor

btm commented Jun 25, 2014

Being a unit test, as we discussed, please add a stub so Resolv.getname doesn't actually go out and make a dns call. ensure that we have at least one test that explicitly checks that we get the ip address when DNS fails too.

@chef-supermarket
Copy link

Hi. Your friendly Curry bot here. Just letting you know that there are commit authors in this Pull Request who appear to not have signed a Chef CLA.

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

@squaresurf
Copy link
Author

@gtmanfred do you mind singing the CLA? That way you get credit and we can use your contributions to this?

@gtmanfred
Copy link
Contributor

Rackspace already approved and added me to the company CLA signature whatever was required. So you should be all good to go.

@squaresurf
Copy link
Author

Thank you @gtmanfred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are these lines here default stubs? I would expect default stubs that ensure any new tests don't have to know to stub Resolv to be up in the top level before block starting at line 21. Individual tests can then override those stubs when they want different behavior.

Copy link
Author

Choose a reason for hiding this comment

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

I added that stub here because, I was under the impression that this was the location to set the default stub. Is line 21 actually the correct location for this stub? This is the first time I've edited an rspec file with shared_examples_for

Copy link
Contributor

Choose a reason for hiding this comment

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

So my english was weird, but the concern is if this stub covers the it "should create rackspace" test which is up in the shared_examples, so we're thinking we need a stub for resolv at the top scoping level, up around line 21 of this file.

The code was taken from an example given by Bryan McLellan.
@chef-supermarket
Copy link

Hi. Your friendly Curry bot here. Just letting you know that there are commit authors in this Pull Request who appear to not have signed a Chef CLA.

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

@squaresurf
Copy link
Author

What needs to happen to get this merged?

@gtmanfred
Copy link
Contributor

I am already on rackspace's cla, so i am not going to sign there.

@btm
Copy link
Contributor

btm commented Oct 22, 2014

@cwebberOps could you look at helping get Rackspace's CCLA moved over to supermarket and their employees attached to it?

@btm
Copy link
Contributor

btm commented Oct 22, 2014

so this is nearly good to go. @gtmanfred is on the old Approved Contributors list as 1814, which is fine.

we do need to make sure we stub out resolv for all of the tests, one way or another. so one of the stubs needs to get moved up to the top scope at the top of the unit test file. If someone can't work that out we'll do so and merge at some point.

@chef-supermarket
Copy link

Hi. I am an automated pull request bot named Curry. There are commits in this pull request whose authors are not yet authorized to contribute to Chef Software, Inc. projects or are using a non-GitHub verified email address. To become authorized to contribute, you will need to sign the Contributor License Agreement (CLA) as an individual or on behalf of your company. You can read more on Chef's blog.

GitHub Users Who Are Not Authorized To Contribute

The following GitHub users do not appear to have signed a CLA:

Please sign the CLA here.

sersut pushed a commit that referenced this pull request Oct 27, 2014
[OHAI-518] remove *.static.cloud-ips.com
@sersut sersut merged commit 0476b12 into chef:master Oct 27, 2014
sersut pushed a commit that referenced this pull request Oct 27, 2014
@squaresurf squaresurf deleted the pr-215-fix branch October 27, 2014 14:05
@thommay thommay added Type: Bug Does not work as expected. Status: Ready for Merge and removed Bug labels Jan 24, 2017
@chef chef locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Bug Does not work as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants