Skip to content

Add Support for H2 Database and Fix #73 #256

Merged
bastelfreak merged 3 commits intovoxpupuli:masterfrom
TJM:issue_254_h2
Jul 9, 2018
Merged

Add Support for H2 Database and Fix #73 #256
bastelfreak merged 3 commits intovoxpupuli:masterfrom
TJM:issue_254_h2

Conversation

@TJM
Copy link
Copy Markdown
Contributor

@TJM TJM commented Jul 3, 2018

Pull Request (PR) description

Add Support for H2 database and add sensible defaults for other db related params.

This Pull Request (PR) fixes the following issues

Fixes #254
Fixes #73

@TJM TJM changed the title Fix #254 Add Support for H2 Database (for testing) and Fix #73 Fix #254 Add Support for H2 Database and Fix #73 Jul 3, 2018
Also Fix voxpupuli#73 - Set appropriate defaults for dbtype and dbdriver based on db.
Comment thread manifests/init.pp
$dbdriver = 'org.postgresql.Driver',
$dbtype = 'postgres72',
$dburl = undef,
Optional[Variant[Integer,String]] $dbport = undef,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

datatypes \o/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For what its worth, I was tempted to use Optional[Stdlib::Port::Unprivileged], but was afraid I would break existing configurations for folks who insist on quoting numbers like '3306', so while this doesn't have as much validation, it was at least "backwards" compatible :)

@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you convert this to an epp() template please?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I figured consistency is better, otherwise we have to convert all the templates to EPP, and if we bother doing that, we should just make it one template and conditionally include the lines as needed instead of having several files that are almost exactly the same.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@bastelfreak What do you think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah good point, let's keep it as is.

@bastelfreak bastelfreak added bug Something isn't working enhancement New feature or request needs-work not ready to merge just yet and removed bug Something isn't working labels Jul 5, 2018
@bastelfreak
Copy link
Copy Markdown
Member

Hi @TJM, thanks for the PR! Can you please take a look at the inline comment I made?

@bastelfreak bastelfreak changed the title Fix #254 Add Support for H2 Database and Fix #73 Add Support for H2 Database and Fix #73 Jul 9, 2018
@bastelfreak bastelfreak removed the needs-work not ready to merge just yet label Jul 9, 2018
@bastelfreak bastelfreak merged commit 425533e into voxpupuli:master Jul 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add H2 database support consolidate db, dbtype, and dbdriver

2 participants