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

Use concat fragments instead of file resource templates for config #64

Merged
merged 1 commit into from
Jul 6, 2016

Conversation

dmsimard
Copy link
Contributor

@dmsimard dmsimard commented Jul 5, 2016

The net effect of this change is zero - it's backwards compatible
and does not result in any change for users.
The concat resources create the files with the same permissions
and the concat fragments take care of inserting the templates with
the same feature to override the template if need be.

However, since concat is now used, consumers of puppet-dns can now
insert their own concat fragments without having to maintain their
own template.
Before this change, if we wanted to diverge slightly from the
template, we would need to "fork" the template and maintain our own.
We could otherwise edit the file with resources like file_line but
this is not idempotent.

Spec test coverage for these bits have been improved a bit and
standardized as part of the commit.

@dmsimard
Copy link
Contributor Author

dmsimard commented Jul 5, 2016

Results for the two templates..

Before:

Notice: /Stage[main]/Dns::Config/File[/etc/named/options.conf]/content: content changed '{md5}0f8469748b8a0403c3d687976370f180' to '{md5}d10d4b5c17906eb720d9965d80d1e98f'
Notice: /Stage[main]/Dns::Config/File[/etc/named.conf]/content: content changed '{md5}c4b43a63125a3d365700e73ad5161b96' to '{md5}0a81a69c667dc73ac1b4133b5a732575'
Notice: /Stage[main]/Dns::Service/Service[named]: Triggered 'refresh' from 1 events

After:

Notice: /Stage[main]/Dns::Config/Concat[/etc/named.conf]/File[/etc/named.conf]/content: content changed '{md5}c4b43a63125a3d365700e73ad5161b96' to '{md5}0a81a69c667dc73ac1b4133b5a732575'
Notice: /Stage[main]/Dns::Config/Concat[/etc/named/options.conf]/File[/etc/named/options.conf]/content: content changed '{md5}0f8469748b8a0403c3d687976370f180' to '{md5}d10d4b5c17906eb720d9965d80d1e98f'
Notice: /Stage[main]/Dns::Service/Service[named]: Triggered 'refresh' from 1 events

@dmsimard dmsimard force-pushed the master branch 3 times, most recently from 127f549 to 2f6d157 Compare July 5, 2016 19:40
@EmilienM
Copy link

EmilienM commented Jul 5, 2016

👍 excellent work here.

@ekohl
Copy link
Member

ekohl commented Jul 5, 2016

I would be open to such a change. I'll let Travis do it's thing, but I think 👍 once it's green.

@iurygregory
Copy link

Awesome work David =D

@dmsimard
Copy link
Contributor Author

dmsimard commented Jul 5, 2016

@ekohl thanks! Looks like travis is green :)

mode => '0640',
}

concat::fragment { 'namedconf_template':
Copy link
Contributor

@domcleal domcleal Jul 6, 2016

Choose a reason for hiding this comment

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

Use the existing naming scheme for fragments, i.e. named.conf+10-main.dns or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@domcleal
Copy link
Contributor

domcleal commented Jul 6, 2016

👍 looks fine now, thanks.

The net effect of this change is zero - it's backwards compatible
and does not result in any change for users.
The concat resources create the files with the same permissions
and the concat fragments take care of inserting the templates with
the same feature to override the template if need be.

However, since concat is now used, consumers of puppet-dns can now
insert their own concat fragments without having to maintain their
own template.
Before this change, if we wanted to diverge slightly from the
template, we would need to "fork" the template and maintain our own.
We could otherwise edit the file with resources like file_line but
this is not idempotent.

Spec test coverage for these bits have been improved a bit and
standardized as part of the commit.
@dmsimard
Copy link
Contributor Author

dmsimard commented Jul 6, 2016

Missed something with my find/replace in the spec tests, should be good now.

@dmsimard
Copy link
Contributor Author

dmsimard commented Jul 6, 2016

@domcleal @ekohl I think we're good now, thanks for your help. Would love to see this in ASAP, it'd unblock an ongoing effort on our end.

@brandonweeks brandonweeks merged commit bd8dc8e into theforeman:master Jul 6, 2016
@brandonweeks
Copy link
Member

@dmsimard merged, thanks!

@EmilienM
Copy link

EmilienM commented Jul 6, 2016

@brandonweeks do you have any ETA before next release?

@mmoll
Copy link
Contributor

mmoll commented Jul 6, 2016

IMHO we can cut a 3.3.1 from current master any time... anybody from @theforeman: any objections?

@ekohl
Copy link
Member

ekohl commented Jul 7, 2016

No objection from my side

@mmoll
Copy link
Contributor

mmoll commented Jul 7, 2016

Released 3.3.1 to the forge.

openstack-gerrit pushed a commit to openstack/puppet-designate that referenced this pull request Jul 7, 2016
- Change file_line resources to concat resources instead to
  concat a fragment within the puppet-dns concat templates
  resources.

This change depends on an upstream change to puppet-dns which
has not yet merged.
theforeman/puppet-dns#64

Change-Id: I499f2c7bfe8330ddb3c3d91d5eadcdad9e64e614
Depends-On: I73145a8992292038ab22824d3a858dcc7193fd35
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.

None yet

7 participants