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

Create ns.pp #53

Merged
merged 13 commits into from
Oct 27, 2015
Merged

Create ns.pp #53

merged 13 commits into from
Oct 27, 2015

Conversation

gilneidp
Copy link
Contributor

It allows to insert NS records into db zone files

It allows to insert NS records into db zone files
@gilneidp gilneidp closed this Jun 30, 2014
@gilneidp gilneidp reopened this Jun 30, 2014
@gilneidp
Copy link
Contributor Author

It is just an adaptation, I didn't find anyway to add NS records using the modules already written, so I did this..

host => $host,
ttl => $ttl,
record => 'NS',
preference => $preference,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why set a preference? This doesn't seem to make sense for a NS record?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I don't need a preference, just a way to automatically insert NS records, and this one worked... I can just erase that line, It's ok...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes please, copied+pasted things that are unnecessary are confusing.

Also I fixed the rspec upgrade breakage, if you post a new diff the tests should pass.

We don't currently test these dns::record wrappers, so I guess I wan't make you write tests for this particular functionality? (would be nice to have)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Already done. I hope it will be util for more people!

Removed unused variable
Inserted NS record option. This option allows to create a NS record into zones.
host => $host,
ttl => $ttl,
record => 'NS',
data => "${data}.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have justification for the ending "." ? I see it in MX, but I don't know if I agree with it.
What if you don't want it to end in a "." ?
Seems like it should be left up to the user if they are going to fully qualify and use a "." or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I didn't think the user could want to have this option, (I never needed to use without ".") but it's simple leave without, I will just add a sample to be used with and without "."
Thanks for notice that!!

Removed "." in $data, so the user can chose by his own put "." or not
Updated option for NS entries
dns::record::ns {
'example.com.':
zone => 'example.com',
# data => ["dns1.example.com.", "dns2.example.com"]; # Two NS for the domain
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like the commented out thing. Just have two examples if you want to show the array and put the comments at the top of the code block.

But if you do do that, be consistent, because you are missing the dot on the second one, which might surprise someone.

Also, use single quotes in your strings for that to follow the puppet style guide. It is a nice to have, and makes your example code look like official code.

Code reorganized in NS records
# Note: Multiple NS can be used as following sample: data => ['dns1.example.com.', 'dns2.example.com.'];
dns::record::ns {
'example.com.':
zone => 'example.com',
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know I'm being a stickler here, but the example source code on the readme should be consistent with other code on the readme as well as the official puppet style guide. Make it look like this

dns::record::ns { 'example.com.':
  zone => 'example.com',
  data => "$hostname.example.com.";  
}

typo correction
Module Install updated
Example Using Hiera to feed the script
@solarkennedy solarkennedy merged commit c8dc35e into ajjahn:master Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants