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

Refactor resources to match packaging configurations #80

Merged
merged 77 commits into from Aug 4, 2017

Conversation

Projects
None yet
3 participants
@therobot
Copy link
Contributor

commented Jul 26, 2017

This PR which is a work in progress for now is created with the intention to fix the naming issues created in previous releases and will wrap better around pdns conventions on how to provide virtual hosting.

therobot added some commits Jul 25, 2017

TDD sysvinit_name
Add a test for sysvinit_name method with correct 
name.

Correct naming scheme for that method for good,
reflecting when parameter is nil (basic case) and
when its named as well.
TDD sysvinit_name for recursor
Doing the same changes for recursor instances on
sysvinit

@therobot therobot self-assigned this Jul 26, 2017

therobot and others added some commits Jul 26, 2017

Virtual hosting on recursor
Adapting debian init script from authoritative to
support virtual hosting on recursor
Recursor helpers update
- Checking for empty?
- powerdns_recursor assumes a ‘recursor’ prefix
for config files other than the default which
should be ‘pdns-recursor.conf’
Updating init resource
- Stop pdns-recursor only when we have a 
default, base-case install (when calling 
pdns_recursor_serv ice with ‘’). To allow updating 
a configuration change of this instance.

- Updating pdns-recursor init script to allow
multiple recursor instances by using a symlink
Updating inspec test and test-recipe
To match the edge case of having a default (“”)
resource.
Deprecated code
- It was used for tests/debug
Adding pidfile option to the daemon lsb script
In order to support multiple instances. The lsb 
daemon script requires —-pidfile option
Improve matching on regular expressions
To better match both pdns/pdns-recursor on 
debian/rhel based distros
Also converting everything to regexp classes
This is a particular case
When we use the default recursor, we want to
stop it before starting it again for the first
time since it grab config changes.
Remove recipe tests
These are covered via integration tests. Additional coverage is still
needed in the library helpers.
Move this folder back one tier
It's assumed we're doing unit tests here.

therobot and others added some commits Jul 28, 2017

More tests and corrections
- Adding tests for names with hyphens
- Correcting some calls in expectation that were
copied from other tests and were calling to the
wrong function
Remove hyphen helper
This is replaced by validation callbacks in the resources
This is no longer required
ChefSpec as of the ChefDK 2.0 release now automatically generates them.
@martinisoft
Copy link
Contributor

left a comment

Glad we got to pair on this and make some much better changes for the future of the cookbook. Less code overall and more consistent results.

if name.empty?
'pdns-recursor'
else
"pdns-recursor-#{name}"

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

According to the documents this shouldn't have the second hyphen and should probably be "pdns-recursor_#{name} correct?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

Which documents? If this is a README issue we will need to update them.


supports 'ubuntu', '>= 14.04'
supports 'debian', '>= 8.0'
supports 'centos', '>= 6.0'

depends 'apt'
depends 'yum'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

Why is yum no longer a dep, but it is still listen in the docs as such?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

That was an error in documentation and I have corrected it. As of Chef 12.14 the yum_repository resources are now a core resource inside of Chef.

@@ -52,8 +46,9 @@
# Has specified in the PowerDNS documentation, a symlink to the init.d script
# "pdns" should be enough for setting up a Virtual instance:
# https://github.com/PowerDNS/pdns/blob/master/docs/markdown/authoritative/running.md#starting-virtual-instances-with-sysv-init-scripts
link sysvinit_script do
link "/etc/init.d/#{sysvinit_name(new_resource.instance_name)}" do
to 'pdns'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

Wouldn't this be much safer with an absolute path?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

Good point. 👍

@@ -44,7 +44,7 @@

property :source, [String, nil], default: 'recursor_service.conf.erb'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

why do these take nil and what happens if I set source nil or cookbook nil in my wrapper instead of defaults? I don't understand the usecase

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

They should not be and a good catch there. I'll fix that in a future commit. 👍

include Pdns::RecursorHelpers
property :instance_name, String, name_property: true, callbacks: {
'should not contain a hyphen' => ->(param) { !param.include?('-') },
}
property :cookbook, [String, NilClass], default: 'pdns'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

Same question as before with setting cookbook or source as nil. I think that would not cause sane behavior unless I am misunderstanding something

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

Yeah that should not be setup that way. Good catch 👍

cookbook new_resource.cookbook
action :create
end

service service_name do
link "/etc/init.d/#{sysvinit_name(new_resource.instance_name)}" do
to 'pdns-recursor'

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

Isn't it safer to set this to an absolute path name?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

Will fix that in a future commit.

context 'with a name' do
let(:instance) { 'foo' }
it 'returns the service name with a virtual instance name' do
expect(subject.sysvinit_name(instance)).to eq('pdns-recursor-foo')

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Jul 31, 2017

Contributor

Shouldn't this be pdns-recursor_foo?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 1, 2017

Contributor

It should not. The name should include the hyphen there. The way PowerDNS does naming in their docs states you should have a hyphen between the instance name and the init script, but don't add a hyphen in the name itself because it splits the name based upon the hyphen in the script name.

martinisoft added some commits Aug 1, 2017

Remove nilclass option and make source optional
This re-works and simplifies the logic to work only if a custom source
has been set.
Remove nil as an option here
It must be defined for Chef to be able to find the template source file
and which cookbook it should look in for it.
Remove nilclass as an option here
Chef won't know where to look and what to look for if it is not defined.

I have pushed all new commits to address the issues. Please re-review.

@martinisoft martinisoft requested a review from onlyhavecans Aug 1, 2017

@onlyhavecans
Copy link
Contributor

left a comment

The documents also need to be updated to note that nil is no longer an option for the updates sources

# https://doc.powerdns.com/md/authoritative/running/#virtual-hosting
link "/etc/init.d/#{sysvinit_name(new_resource.instance_name)}" do
to '/etc/init.d/pdns'
not_if { new_resource.instance_name.empty? || new_resource.property_is_set?(:source) }

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Aug 2, 2017

Contributor

Why is this not_if different than the one for recursor? Below it's just not_if { new_resource.instance_name.empty? }

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 2, 2017

Contributor

The recursor package for sysvinit does not support the virtual naming and has the runfile bug which has not been patched upstream.

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 2, 2017

Contributor

Basically what we want to do for the authoritative is defer to their packaged init versus attempting to override it by default. If you actually do override it, you're signing on the dotted line that you know what you are doing and we stop attempting to symlink anything. There is something to be said for supporting the symlinking, but I'd want someone to propose a patch with a use case then.

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Aug 2, 2017

Contributor

Understood, then the comment about updating the documentation on what is allowed by the resources is my only left concern. This also might want to be very clearly spelt out since it caught me off guard too

variables: new_resource.variables
)
action :create
only_if { new_resource.property_is_set?(:source) }

This comment has been minimized.

Copy link
@onlyhavecans

onlyhavecans Aug 2, 2017

Contributor

Why does this use an only_if for the property? wouldn't we always want this even if we are just linking below?

This comment has been minimized.

Copy link
@martinisoft

martinisoft Aug 2, 2017

Contributor

This is only if you want to replace the packaged init script with a new template

I have responded to both bits of feedback and pushed 3 final commits to correct an inconsistency with variables.

@martinisoft martinisoft requested a review from onlyhavecans Aug 2, 2017

@martinisoft martinisoft changed the title Service naming for good Refactor resources to match packaging configurations Aug 4, 2017

@martinisoft martinisoft merged commit 48f8a52 into master Aug 4, 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 bugfix/service_naming_for_good branch Aug 4, 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.