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

fixed params.pp for rhel 7 and added fixes for concat issues #84

Merged
merged 5 commits into from
Feb 21, 2015

Conversation

ITBlogger
Copy link
Contributor

No description provided.

@solarkennedy
Copy link
Collaborator

Obviously we were missing test coverage to catch the concat bug.

Can you take the tests from https://github.com/ajjahn/puppet-dns/pull/80/files
and add them into this PR to prove that this change actually fixes the issue?

Additionally travis is complaining about style guide violations.

@ITBlogger
Copy link
Contributor Author

Yeah, I'll add that stuff in...and fix the style issues...what's funny is that some of them are from the current build.

@ITBlogger
Copy link
Contributor Author

I also found some other issues that need to be addressed, so I'll get those added too. For instance, bind's config file needs includes for the local.conf and local.options.conf

@solarkennedy
Copy link
Collaborator

Ok, try to limit the PR to one issue per PR, that makes it easy to ship and reason about changes.

Style guide violations... shouldn't be on the current build. Jenkins says the current build is passing. Are you on the same master as upstream?

@ITBlogger
Copy link
Contributor Author

Yes...I just know that I didn't make any changes to key.pp and I just
checked...it should be failing on the current build. Lines 64 and 65 have
one more space than is needed for the arrows.

--Alex

On Thu, Feb 19, 2015 at 7:44 AM, Kyle Anderson notifications@github.com
wrote:

Ok, try to limit the PR to one issue per PR, that makes it easy to ship
and reason about changes.

Style guide violations... shouldn't be on the current build. Jenkins says
the current build is passing. Are you on the same master as upstream?


Reply to this email directly or view it on GitHub
#84 (comment).

@solarkennedy
Copy link
Collaborator

As far as I can tell, the current master (59db4a4) conforms to the style guide:
https://github.com/ajjahn/puppet-dns/blob/master/manifests/key.pp#L63-L65

Please pull master to make sure you are 100% up to date.

@ITBlogger
Copy link
Contributor Author

The current master does not conform to style guide...currently it's:

  concat::fragment { "${name}.key-secret":
    order   => 2,
    source  => "${cfg_dir}/bind.keys.d/${name}.secret",
  }

and should be:

  concat::fragment { "${name}.key-secret":
    order  => 2,
    source => "${cfg_dir}/bind.keys.d/${name}.secret",
  }

@ITBlogger
Copy link
Contributor Author

By the way, the problem with concat wasn't a bug in concat, but rather a problem with the puppet manifests. The manifests were written to dump files to a directory that didn't exist.

@ITBlogger
Copy link
Contributor Author

Yay! The build passed!

let(:facts) {{ :osfamily => 'RedHat', :concat_basedir => '/dne', :define_fact => "" }}

context 'When given connected records that depend on each other' do
it { should compile }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This test isn't testing anything yet because testhost.example.com is not yet defined.

Can you copy in the file from
https://github.com/ajjahn/puppet-dns/pull/80/files#diff-4f9b2b877cece59826df5b65a4980421R1

Adding that file and ensuring it can compile will be a definitive test that this all integrates correctly and the dependency bug is fixed, and will stay fixed.

@ITBlogger
Copy link
Contributor Author

Yeah, LOL, spec/fixtures was in .gitignore so it didn't get added to the previous commits

@ITBlogger
Copy link
Contributor Author

Yay, the build passed!

@solarkennedy
Copy link
Collaborator

Great! Thank you very much. You have solved the concat issue and ensured that it won't happen again!

solarkennedy added a commit that referenced this pull request Feb 21, 2015
fixed params.pp for rhel 7 and added fixes for concat issues
@solarkennedy solarkennedy merged commit f32e3fd into ajjahn:master Feb 21, 2015
@solarkennedy
Copy link
Collaborator

And lint fixes!

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