Skip to content
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

Authoritative Resource #49

Merged
merged 86 commits into from May 18, 2017

Conversation

Projects
None yet
3 participants
@therobot
Copy link
Contributor

commented Mar 21, 2017

This PR holds the discussion and review of the authoritative resource. It follows it's sister PR which addresses the recursor resource and mirrors much of it's principles and functionality.

The plan it's to have the same 3 resources than the recursor plus one additional that will configure different backends.

I will expand this section once is ready for review.

@therobot

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2017

As per a conversation with @onlyhavecans I will re-add nominal support for the systemd OSes (ubuntu 16.04, debian 8, centos 7.2) albeit it'll be an incomplete one since I won't work on specific systemd resources until a 3.1 version of the cookbook.

@martinisoft
Copy link
Contributor

left a comment

I found a lot of grammatical issues in the README along with a bigger issue in the metadata. Once those are corrected I'll give it another look. Overall really awesome changes that are really well documented! 👍

README.md Outdated
@@ -4,49 +4,230 @@ Provides resources for installing and configuring both PowerDNS authoritative an

## Requirements

IMPORTANT: Please read the Compatibility Notes version below since there is breaking changes between 2 and 3 versions of this cookbook.
IMPORTANT: Please read the Deprecations and Compatibility Notes sections below since there is breaking changes between 2 and 3 versions of this cookbook.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

s/is/are/ and s/breaking changes between 2 and 3 versions/breaking changes between version 2 and 3/

README.md Outdated

### Deprecations

- The recipe and attribute based way of setting different PowerDNS installs is completely deprecated, there is not attributes in the newest version of this cookbok and just a simple empty `default` recipe.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

s/is not /are no/

README.md Outdated

- The recipe and attribute based way of setting different PowerDNS installs is completely deprecated, there is not attributes in the newest version of this cookbok and just a simple empty `default` recipe.
- `pdnsrecord` and `domainrecord` resources have been deprecated since they were coupled with sqlite3 backend.
- Ubuntu 12.02 support has been removed, if you want this platform to be supported PRs are welcome, see Contributing section at the end of this document.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

I'd not offer to do a PR here and would reword this to:

"Ubuntu 12.02 support has been removed because it is End of Life and no longer supported by Chef and PowerDNS."

README.md Outdated

### Compatibility Notes

**This cookbook is being completely rewritten, transitioning from an attribute centric design to a newer resource based design. The current 3.0 version the resource only supports recursors, being the authoritative server the next feature, destined for the 3.1 release which will be released soon. If you want to keep your authoritative PowerDNS installs pin your cookbook to the latest 2.5.0 version.**
**This cookbook is being completely rewritten, transitioning from an attribute centric design to a newer resource based design.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

s/is being/has been/ and s/attribute centric/attribute and recipe based/

README.md Outdated
* postgres (for the PostgreSQL backend)
## Usage
Use the `pdns_recursor_install`, `pdns_recursor_config`, and `pdns_recursor_service` resources in your wrapper cookbooks to install, configure, and define PowerDNS recursors. Set the different properties on the resources according to your install and configuration needs. You can see a good example on this on `test/cookbooks/pdns_test/recipes_recursor_install_single.rb`
Combine the different resources in order to install, configure and manage your PowerDNS instances. This is a list of resouces that can be used:

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

s/configure and/configure, and/ for Oxford comma compliance ™️

This comment has been minimized.

Copy link
@therobot

therobot May 3, 2017

Author Contributor

How could I missed that! 😱😱😱

README.md Outdated
@@ -105,6 +302,9 @@ Create a PowerDNS recursor configuration named 'my-recursor' in your wrapper coo
variables(client-tcp-timeout: '20', loglevel: '5', network-timeout: '2000')
end
## Contributing
We are happy to accept contributions to this cookbook in form of bug fixes, new backends, init services, or platform support. We believe that barriers to contributing to this cookbook has been lowered since the release of the 3.0, resource based version. Please use the normal PR workflow for contributing. We will favour PRs with tests for the changes, we use both Chefspec and Inspec testing framewowrks.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

To better comply with new Supermarket linting, I'd extract this into a CONTRIBUTING.md document and note it here. I'd also make a testing section note and put that in a TESTING.md document so people know how they can test this cookbook themselves.

# Cookbook Name:: pdns
# Libraries:: authoritive_helpers
#
# Copyright 2014, Aetrion, LLC.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

Go ahead and update this copyright to 2014-2017 Aetrion LLC. dba DNSimple

@@ -1,6 +1,6 @@
#
# Cookbook Name:: pdns
# Libraries:: helpers
# Libraries:: recursor_helpers
#
# Copyright 2014, Aetrion, LLC.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

Go ahead and update this copyright to 2014-2017 Aetrion LLC. dba DNSimple

metadata.rb Outdated
@@ -1,18 +1,17 @@
name 'pdns'
name 'chef-pdns'

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

Why did this change? Our cookbook and supermarket name is pdns. If you uploaded this now it would create chef-pdns instead of pdns.

This comment has been minimized.

Copy link
@therobot

therobot May 3, 2017

Author Contributor

To close this issue: #47

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans May 9, 2017

Contributor

This goes against supermarket naming conventions. chef- is only for cookbooks owned by chef- organization technically. THis needs to stay pdns or we can file with supermarket/chef to get dnsimple- space.

@@ -0,0 +1,31 @@
<% if @config_dir %>

This comment has been minimized.

Copy link
@martinisoft

martinisoft Apr 13, 2017

Contributor

You may want to consider putting in comments for this file if the config format supports commented lines to note that the file is managed by chef and to not manually edit it.

This comment has been minimized.

Copy link
@therobot

therobot May 3, 2017

Author Contributor

👍

@therobot therobot dismissed stale reviews from onlyhavecans and martinisoft May 3, 2017

Requesting a new review

@therobot therobot requested review from martinisoft and onlyhavecans May 3, 2017

@onlyhavecans
Copy link
Contributor

left a comment

We can't ship the cookbook with that name change

metadata.rb Outdated
@@ -1,18 +1,17 @@
name 'pdns'
name 'chef-pdns'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans May 9, 2017

Contributor

This goes against supermarket naming conventions. chef- is only for cookbooks owned by chef- organization technically. THis needs to stay pdns or we can file with supermarket/chef to get dnsimple- space.

maintainer 'Aetrion, LLC DBA DNSimple'
maintainer_email 'ops@dnsimple.com'
license 'Apache 2.0'
description 'Installs/Configures PowerDNS Recursor and Authoritative server'
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
version '2.5.0'
source_url 'https://github.com/dnsimple/pdns'
issues_url 'https://github.com/dnsimple/pdns/issues'
source_url 'https://github.com/dnsimple/chef-pdns'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans May 9, 2017

Contributor

changing the name here is fine though

@onlyhavecans
Copy link
Contributor

left a comment

LGTM at this point!

@martinisoft
Copy link
Contributor

left a comment

Overall pretty big PR and really solid work with some notes about what I think should change. All of this code is making me wonder if we should split the cookbook into a powerdns_recursor and a powerdns_server to simplify things more. While they are related projects, they provide entirely different functions.

end
end

def backend_package_per_platform

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

This method makes me sad and want to cry. Could it be a case statement at least? This is insanely hard to read.

I'd consider this as a nicer, easier to read alternative with extracted methods and case statements:

def backend_package_per_platform
  case node['platform_family']
  when 'debian'
    backend_package_debian
  when 'rhel
    backend_package_rhel
  end
end

def backend_package_debian
  case new_resource.instance_name
  when 'geo'
    'pdns-backend-geo'
   end
end

def backend_package_rhel
  case new_resource.instance_name
  when 'geo'
    'pdns-backend-geo'
   end
end

This comment has been minimized.

Copy link
@therobot

therobot May 17, 2017

Author Contributor

I disagree on this one, I tried to find the best way to cover all the combination of cases in a way that it's clear to understand. To me (because this is to an extent subjective) this way of presenting the different combinations between backend modules and available platforms is way clearer than the alternative you present.

The main reason is that it covers all the cases at once glance with very simple ruby logic. The second reason is because there is no need to branch from one method to the next. Compare this with having a total of three methods and god nows how many case branches for that, with all the syntax involved in case statements the space it will take on screen is enormous probably requiring to jump up and down the file in the text editor.

I haven't invented anything new here, this method is directly inspired in how the httpd cookbook handles all the different cases between platforms, apache versions and apache modules.

end
end

def default_authoritative_run_user

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

Since the cases here are identical, I'd just return 'pdns' for now in this method. Why add logic where it adds no value unless you intend to return nil.

end

property :instance_name, String, name_property: true
property :cookbook, [String,nil], default: 'pdns'

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

I would just make this a String type with the default. Can't really see a nil use case here.


property :instance_name, String, name_property: true
property :cookbook, [String,nil], default: 'pdns'
property :source, [String,nil], default: 'authoritative.init.debian.erb'

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

Same as the cookbook line above. This should just be a String with default.

end

property :instance_name, String, name_property: true
property :cookbook, [String,nil], default: 'pdns'

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

As I mentioned before, nil does not make sense here as a property option.


property :instance_name, String, name_property: true
property :cookbook, [String,nil], default: 'pdns'
property :source, [String,nil], default: 'authoritative.init.rhel.erb'

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

As I mentioned before, nil does not make sense here as a property option.

@@ -1,6 +1,8 @@
package 'bind-utils' if node['platform_family'] == 'rhel'

This comment has been minimized.

Copy link
@martinisoft

martinisoft May 11, 2017

Contributor

I'd use an only_if here and set the action to 'install'

@therobot therobot merged commit f385030 into master May 18, 2017

@martinisoft martinisoft deleted the enhancement/authoritative_resource branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.