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

Support PostgreSQL #8

Merged
merged 3 commits into from Jan 20, 2016

Conversation

Projects
None yet
4 participants
@ekohl
Copy link
Member

ekohl commented Oct 10, 2015

Todo:

  • Update example config
  • Handle mysql / pg dependencies
  • Pin pg to <= 0.18.0 on ruby 1.8.7

@ekohl ekohl referenced this pull request Oct 10, 2015

Closed

postgresql support #5

@ekohl ekohl force-pushed the postgresql branch 2 times, most recently from 20131c8 to ec4a88b Oct 10, 2015

@ekohl ekohl changed the title [WIP] Support PostgreSQL Support PostgreSQL Oct 10, 2015

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Oct 10, 2015

The current state is that it all works (yay integration tests), but it could use a good review. In particular the dependencies need attention. I'm also considering changing the mysql config to something nested, but currently I'm staying compatible with the released version.

@domcleal mind reviewing?

@ekohl ekohl force-pushed the postgresql branch from ec4a88b to 30e3682 Oct 11, 2015

@mmoll

This comment has been minimized.

Copy link
Member

mmoll commented Oct 12, 2015

I wouldn't pin pg, because on systems (e.g. FreeBSD or the upcoming Ubuntu LTS) that bring a newer pg packaged, it won't work because of this, despite 0.18.3 would be perfectly compatible.

Rather I'd use < 0.19 for now and if needed specify per Ruby version in the Gemfile like in http://git.io/vClgM

@powerdns_pdnssec = options[:powerdns_pdnssec] || false
unless options[:powerdns_backend]
# TODO: require the backend?
options[:powerdns_backend] = 'mysql' if option[:powerdns_mysql_hostname]

This comment has been minimized.

@ekohl

ekohl Oct 12, 2015

Member

options

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Oct 13, 2015

I would suggest moving out validation of parameters out of constructors so it could be executed during plugin initialization: smart proxy will have a chance to disable the provider and dns plugin and log the issues in this case. For an example see puppet_proxy (https://github.com/theforeman/smart-proxy/blob/develop/modules/puppet_proxy/puppet_plugin.rb#L18 and https://github.com/theforeman/smart-proxy/blob/develop/modules/puppet_proxy/ssl_configuration_validator.rb).

Now that support for dependency injection is in smart-proxy, this provider can benefit from it. puppet_proxy is using dependency injection to "wire" itself during initialization to support puppet setup it interfaces with. Some of the points of interest there:

I'm using the same approach for configuring of providers in DHCP module (although it's a PR atm).

I'd need to introduce a few changes to the DNS module for it to be able to rely on dependency injection for provider resolution, but they wiould be quite minor and could be done quickly. Let me know if you you have questions or would like a walk through dependency injection usage in puppet and dhcp modules.

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Oct 13, 2015

@witlessbird ty for the many hints. I'll try to apply them and get back to you either with a solution or questions :)

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Oct 14, 2015

@witlessbird the dependency injection does look like a fitting solution, but I'll gladly take you up on a walk through.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Oct 15, 2015

@ekohl: would some time on Monday work?

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Oct 15, 2015

@witlessbird that would work well since I took monday off.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Oct 19, 2015

@ekohl: Failure at https://github.com/ekohl/smart_proxy_dns_powerdns/blob/b4e1f577272f6c5601db1ade4f212a21620a20e9/test/unit/dns_powerdns_configuration_validator_test.rb#L26 could be due to setting powerdns_backend in Proxy::Dns::Plugin.settings, but attempting to read it from Proxy::Dns::Powerdns::Plugin.settings.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Nov 3, 2015

@ekohl: how goes? Do you need any help with this?

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Nov 3, 2015

@witlessbird other than a time machine I don't think I need help at this moment.

@ekohl ekohl force-pushed the postgresql branch 3 times, most recently from d88b53b to 4394fcd Jan 18, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 18, 2016

I finally made time to rewrite the PR to dependency injection.

Will need to look at unit test failures on 1.9.3. If a more experienced ruby developer can help me, I'd greatly appreciate it.

I'm also wondering why I needed to modify the integration tests. This could be a regression in the API or just some error on my side.

backend = settings[:powerdns_backend]

unless backend
settings[:powerdns_backend] = 'mysql' if settings[:powerdns_mysql_hostname]

This comment has been minimized.

@dmitri-d

dmitri-d Jan 18, 2016

Member

I would suggest not to modify plugin settings -- they should be treated as read-only (probably will be enforced in the future). I would also to force users to provide values for critical settings (such as powerdns_backend) instead of deriving them from other values.

This comment has been minimized.

@ekohl

ekohl Jan 18, 2016

Member

I initially considered this as an easy upgrade path, but will reconsider this.

end

def validate_presence(*setting_names)
# TODO: Use a PluginValidator?

This comment has been minimized.

@dmitri-d

dmitri-d Jan 18, 2016

Member

You could, although the interface might feel a bit weird...

This comment has been minimized.

@ekohl

ekohl Jan 18, 2016

Member

I was thinking about this and it's conditional presence. It may be a weird API.

@ekohl ekohl force-pushed the postgresql branch 3 times, most recently from d76058a to 035d202 Jan 18, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

@witlessbird updated to address your comments.

I am running into issues on 1.8.7 and 1.9.3 with test failures. load_test_settings doesn't appear to have any effect on 1.9.3 and 1.8.7 just fails on every test.


class Proxy::Dns::DependencyInjection::Dependencies
case Proxy::Dns::Powerdns::Plugin.settings[:powerdns_backend]
when 'mysql' then

This comment has been minimized.

@dmitri-d

dmitri-d Jan 19, 2016

Member

No need for 'then' here and below.

when 'mysql' then
validate_presence :powerdns_mysql_username, :powerdns_mysql_password, :powerdns_mysql_database
when 'postgresql' then
validate_presence :powerdns_postgresql_connection

This comment has been minimized.

@dmitri-d

dmitri-d Jan 19, 2016

Member

Out of curiosity, why do you have settings for username and password for mysql, but none for pg, which supports those as well (https://github.com/ged/ruby-pg/blob/master/lib/pg/connection.rb#L24)? If you decide to add these for pg, you could keep the same setting names for both dbs, which might make validation a bit easier...

This comment has been minimized.

@ekohl

ekohl Jan 19, 2016

Member

Because I was unsure about dealing with optional parameters. The connection string is easier because it can also handle socket connections.

end

def test_initialize_missing_backend
Proxy::Dns::Powerdns::Plugin.load_test_settings(

This comment has been minimized.

@dmitri-d

dmitri-d Jan 19, 2016

Member

Probably would be easier to test this if you passed settings as a parameter to the #validate_settings! method or the class constructor. Then in the after_activation block: ::Proxy::Dns::Powerdns::ConfigurationValidator.new.validate_settings!(settings)

This comment has been minimized.

@ekohl

ekohl Jan 19, 2016

Member

Good idea. That would also solve the problem that load_test_settings doesn't tear down.

This comment has been minimized.

@dmitri-d

dmitri-d Jan 19, 2016

Member

Nods, forgot to mention that. Although I'm looking at it now...

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 19, 2016

Test failures on 1.8.7 is due to you using test-unit gem (I think it assumes that minitest is available?). If you scope it to RUBY_VERSION > '1.8.7' those should go away.

@ekohl ekohl force-pushed the postgresql branch from 035d202 to 63f465b Jan 19, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

@witlessbird the tests for 1.8.7 now fail like the 1.9.3 which is the load_test_settings invocation, so that's progress.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 19, 2016

The other test failures are due to openstruct differences between various ruby platforms: doesn't look like #[] method is defined for older rubies, switching to ".field_name" should fix this.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 19, 2016

I don't have power_dns to test against, but the code looks good!

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

The other test failures are due to openstruct differences between various ruby platforms: doesn't look like #[] method is defined for older rubies, switching to ".field_name" should fix this.

Not sure I understand what you mean by this.

I don't have power_dns to test against, but the code looks good!

Cool. There are integration tests (which I only manually ran, still have to fix a travis build for it) which I used to verify functionality. Once the tests pass, I'll merge this and work to a 0.2.0 release in time for my talk at cfgmgmtcamp.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 19, 2016

Apologies for not being clear - plugin settings class is derived from ruby's Openstruct. In earlier versions of ruby, '[]' method isn't defined for Openstruct, to access individual keys one has to use key name as the method name to access it's value:

o = OpenStruct.new(:first => 1, :second => 2)
o.first # returns 1
o.second # return 2

@ekohl ekohl force-pushed the postgresql branch 2 times, most recently from 322a079 to e4b9ba1 Jan 19, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

That works. Now the only remaining point is dependencies. I'd prefer not to require the mysql gem for postgres users and the reverse. Is this feasible? How will this translate in RPM/deb packaging?

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

By fixing the unit tests I broke the integration tests. Will look into it.

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 19, 2016

As you are not relying on plugin initializer to load provider's dependencies, you could put pg and mysql gems into separate groups and then use bundler install --without mysql_group to install pg only (for example). I'm not sure what's a good way to handle this with rpm packages... Possibly creating separate packages for mysql- and pg-based provider?

@ekohl ekohl force-pushed the postgresql branch from e4b9ba1 to 1577eae Jan 19, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 19, 2016

@witlessbird I'm having a bit of trouble with the ConfigurationValidator and settings. Since I can't use it as an array, how do I programatically get a setting from it?

def validate_settings!(settings)
validate_choice(settings, :powerdns_backend, ['mysql', 'postgresql', 'dummy'])

case settings[:powerdns_backend]

This comment has been minimized.

@dmitri-d

dmitri-d Jan 20, 2016

Member

you have to use setting.powerdns_backend instead.

end

def validate_choice(settings, setting, choices)
value = settings[setting]

This comment has been minimized.

@dmitri-d

dmitri-d Jan 20, 2016

Member

try using settings.send(setting) here instead.


def validate_presence(settings, names)
names.each do |name|
value = settings[name]

This comment has been minimized.

@dmitri-d

dmitri-d Jan 20, 2016

Member

try settings.send(name)

@ekohl ekohl force-pushed the postgresql branch from 1577eae to 6793c34 Jan 20, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 20, 2016

@witlessbird that works, thanks.

@@ -20,4 +20,5 @@ Gem::Specification.new do |s|
s.add_development_dependency('mocha')

s.add_dependency('mysql2')
s.add_dependency('pg')

This comment has been minimized.

@domcleal

domcleal Jan 20, 2016

The alternative is to leave both deps off, put them only in Gemfile and state that one or the other has to be installed as well for it to work. I don't have any preference, it'll only require client libraries I suppose for both if you keep both here.

Optionally when packaging the plugin you could have extra subpackages for the respective backends if you wished to abstract it out.

This comment has been minimized.

@ekohl

ekohl Jan 20, 2016

Member

Then at first I'll just hard require both and leave it as a future improvement.

@ekohl ekohl merged commit 6793c34 into master Jan 20, 2016

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

@ekohl ekohl deleted the postgresql branch Jan 20, 2016

@ekohl

This comment has been minimized.

Copy link
Member

ekohl commented Jan 20, 2016

Thanks @witlessbird for the help. I owe you a beer so if you're at FOSDEM/cfgmgmtcamp do collect it :)

@dmitri-d

This comment has been minimized.

Copy link
Member

dmitri-d commented Jan 20, 2016

@ekohl: np! Yep, planning to be at both, we'll have a chance to catch up then...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment