New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace fog with dnsimple and rewrite to Chef 12.5+ resource #46

Merged
merged 193 commits into from Apr 23, 2017

Conversation

@martinisoft
Member

martinisoft commented Jan 11, 2017

This is a work in progress PR that completely replaces the fog based code with our own gem. It also drops support for Chef 11 because Chef 13 will be out in April, which sunsets its support officially.

martinisoft and others added some commits Jun 9, 2016

@martinisoft

This comment has been minimized.

Member

martinisoft commented Apr 17, 2017

This is ready for review @onlyhavecans and @therobot

I have added update support as well and additional tests around that functionality. There can be more room for re-factors down the road, but I feel pretty good about this getting out there. We can review together this week and get a 2.0 release out the door and patch in additional testing and cleanup as we go. 👍

def dnsimple_client
require 'dnsimple'
@dnsimple_client ||= Dnsimple::Client.new(

This comment has been minimized.

@weppos

weppos Apr 18, 2017

Member

I suggest to also add the user_agent option with value:

user_agent: "chef-dnsimple VERSION"

Assuming you have a constant that holds the version of the cookbook:

user_agent: "chef-dnsimple #{VERSION}"

That will help to analyse the request logs.

This comment has been minimized.

@martinisoft

martinisoft Apr 19, 2017

Member

As far as I know there isn't an easy way to access the version number or do the constant pattern that rubygems tend to do for version numbering. I'll ask around to see if this can be done. If not, then we can make a note in the release guide about updating both the metadata file and the version number noted in the client so we report it correctly.

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

run_context.cookbook_collection.dnsimple.version should be the version number IIRC

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

I just gave it a shot so I will check the logs on Sandbox to see if it comes through. 👍

name: inspec
attributes:
dnsimple_token: <%= ENV['DNSIMPLE_ACCESS_TOKEN'] %>
test_domain: dnsimple.net

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

.xyz is the testing domain

dnsimple:
access_token: <%= ENV['DNSIMPLE_ACCESS_TOKEN'] %>
base_url: https://api.sandbox.dnsimple.com
test_domain: dnsimple.net

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

.xyz

dnsimple:
access_token: <%= ENV['DNSIMPLE_ACCESS_TOKEN'] %>
base_url: https://api.sandbox.dnsimple.com
test_domain: dnsimple.net

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

.xyz

README.md Outdated
[![Build Status](https://travis-ci.org/dnsimple/chef-dnsimple.png?branch=master)](https://travis-ci.org/dnsimple/chef-dnsimple)
[![Build Status](https://jenkins-01.eastus.cloudapp.azure.com/job/dnsimple-cookbook/badge/icon)](https://jenkins-01.eastus.cloudapp.azure.com/job/dnsimple-cookbook/)
## Requirements
* A DNSimple account at https://dnsimple.com
* Chef 12 or newer
* A [dnsimple][] account

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

Because of the size of this document and the various links can we put all links inline instead of bottom of document to help prevent dropped links?

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

Agreed and done 👍

README.md Outdated
## Deprecation Warning

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

I'd leave a big huge "THIS IS NOT COMPATIBLE WITH 1.X EVEN A LITTLE" Deprecation warning at the top.
Just to highlight how different this is to prevent confusion to casual onlookers.

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

I slapped a neon sign on it. 🎆

def dnsimple_client
require 'dnsimple'
@dnsimple_client ||= Dnsimple::Client.new(

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

run_context.cookbook_collection.dnsimple.version should be the version number IIRC

action :create do
if @current_resource.exists
Chef::Log.info "#{@new_resource} already exists - nothing to do."

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

Why is this not prefixed with DNSimple: WHen the others are?

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

I've corrected this inconsistency. 👍

Chef::Log.info "DNSimple: created #{new_resource.type} record for #{new_resource.name}.#{new_resource.domain}"
end
rescue Dnsimple::RequestError => e
raise "Unable to complete create record request. Error: #{e.message}"

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

Should raise also have the DNSimple: prefix?
dnsimple_provider.rb also has some raises that do not have prefix.

metadata.rb Outdated
recipe 'dnsimple', 'Installs fog gem to use w/ the dnsimple_record'
chef_version '>= 12.8'
gem 'dnsimple'
supports 'amazon'

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

I like making %w[ amazon centos ect ].each do |os| supports os` blocks for this.

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

Resolved 👍

describe Chef::Provider::DnsimpleRecord do
before(:each) do
@node = stub_node(platform: 'ubuntu', version: '14.04') do |node|
node.default['dnsimple']['version'] = '1.2.3'

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 20, 2017

Contributor

What does this provide? I don't see where it is used or required

This comment has been minimized.

@martinisoft

martinisoft Apr 20, 2017

Member

Think that was leftover from the original cookbook that had the gem version so I have since removed it. 👍

martinisoft added some commits Apr 20, 2017

Cleanup this stub
The version attribute is gone since its a resource now.
Add user agent in with cookbook version number
This will help us track usage

I've made the required changes and them some, puh-puh-puhleeease re-review? ^_^

@martinisoft martinisoft requested review from weppos and onlyhavecans Apr 23, 2017

@therobot

Looks good to me, there is a lot of great work here, congrats @martinisoft

platforms:
- name: ubuntu-14.04
- name: ubuntu-16.04
- name: centos-6.8
- name: centos-7.3

This comment has been minimized.

@onlyhavecans

onlyhavecans Apr 23, 2017

Contributor

why is freebsd not here?

This comment has been minimized.

@martinisoft

martinisoft Apr 23, 2017

Member

I missed adding this to the docker side. Couldn't easily test on limited 🇫🇷 internets. Will just push a commit now to test via CI.

This comment has been minimized.

@martinisoft

martinisoft Apr 23, 2017

Member

Ok so it turns out FreeBSD on Docker on Linux is not really a stable thing so it is not tested via CI. The images out there that work can only be run on a FreeBSD host.

martinisoft added some commits Apr 23, 2017

This isn't actually a supported platform
It's complicated, but we simply can't test this via Docker :(

@martinisoft martinisoft merged commit f4ffb61 into master Apr 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@martinisoft martinisoft deleted the deprecation/replace_fog_with_dnsimple branch Oct 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment