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

Change to make the cookbook compatible with CentOS/RHEL #36

Merged
merged 6 commits into from Sep 13, 2016

Conversation

Projects
None yet
4 participants
@stromp
Copy link
Contributor

commented Aug 17, 2016

No description provided.

default['pdns']['recursor']['config_dir'] = '/etc/powerdns'
default['pdns']['recursor']['config']['config_dir'] = '/etc/powerdns'
if node[:platform_family].include?("rhel")
default['pdns']['recursor']['config_dir'] = ' /etc/pdns-recursor'

This comment has been minimized.

Copy link
@therobot

therobot Aug 19, 2016

Contributor

There is an extra whitespace here.

@therobot

This comment has been minimized.

Copy link
Contributor

commented Aug 19, 2016

Hi @stromp thanks for your efforts on contributing to this cookbook.

This looks good to me and I'm 👍 on merging, but I'd like to see at least basic testing support on this platform. For the beggining: adding support for Centos/RHEL on .kitchen.yml and passing the serveral suites is an importante thing to have before I can merge this.

Thanks.

@martinisoft

This comment has been minimized.

Copy link
Contributor

commented Aug 20, 2016

I'll echo @therobot's comments here. Once this branch is updated and corrected we can do another formal code review to get this merged. Thanks again for submitting this pull request!

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2016

@therobot @martinisoft
I added the basic tests for centos 7.2 with the recursor recipe.
The tests are succesful

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2016

@martinisoft @therobot

Hello,

Is this pull request okay to be merged? Or do I need to do something else?

@martinisoft

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

@stromp I can take another look with @onlyhavecans today since @therobot is out on vacation this week.

@onlyhavecans

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

@stromp Why is CentOS disabled in almost all of the test suites?

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

@onlyhavecans @martinisoft

Hi David,

The modifications in my pull request have only added centos support for the
recursor package. This is why I only test the recursor package.

Best regards,

Sjoerd Tromp

On Tuesday, 30 August 2016, David Aronsohn notifications@github.com wrote:

@stromp https://github.com/stromp Why is CentOS disabled in almost all
of the test suites?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#36 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHTY_Xm_QeoSgad-F1sRg-Od_QpsM6iHks5qlH3RgaJpZM4JmirZ
.

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2016

@onlyhavecans @martinisoft

was this answer okay for you?

@martinisoft

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2016

Hi @stromp

We would prefer you test all configurations supported by this cookbook in order to avoid the possibility of this causing a break for others.

metadata.rb Outdated
@@ -4,7 +4,7 @@
license 'Apache 2.0'
description 'Installs/Configures pdns'
long_description IO.read(File.join(File.dirname(__FILE__), 'README.md'))
version '2.2.1'
version '2.2.2'

This comment has been minimized.

Copy link
@martinisoft

martinisoft Sep 6, 2016

Contributor

We would prefer to do the version bumping after a merge, could you please revert this change?

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2016

@martinisoft I removed the version bump

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2016

@onlyhavecans @martinisoft

Can it be merged now? If more things need to be adjusted, please say so.

@therobot

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@stromp in order to merge this branch we need that all the configurations that we run tests on in ubuntu pass tests in CentOS. This is:

  • recursor_package
  • recursor_source
  • authoritative_bind_package
  • authoritative_bind_source
  • authoritative_pgsql_package
  • authoritative_pgsql_source
  • authoritative_pipe_package
  • authoritative_pipe_source
  • slave_package
  • slave_source

The reason is that by the moment one of us merges your branch we would have to provide support for CentOS/RHEL in the future, and we rather have a solid foundation instead just a single isolated case.

Thanks.

@stromp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2016

@therobot @onlyhavecans @martinisoft

For my purposes it's to much time investment to make the whole cookbook compatible with CentOS (which would be needed to pass the tests). I can't justify this to my employer. In that case I will keep working from a fork or solve this with a wrapper cookbook until someone else supplies complete CentOS support. Although I do find it a pity if other people can't benefit from the effort up till now.

@onlyhavecans

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2016

@stromp I understand where you are coming from. We are mostly worried about other users who would want to use the cookbook and find the very partial support to be very frustrating. However I agree it's not ok to throw out the contribution. I'll run the tests and try to get this merged with a large caveat note.

@onlyhavecans onlyhavecans merged commit 22b8000 into dnsimple:master Sep 13, 2016

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.