Use // to specify postgresl database

[ehelms]

No description provided.

[ehelms]

Testing to see if this solves all of the issues I am seeing before I merge it.

[Klaas-]

@ehelms this is a good point to use the spec tests, I've been meaning to do that in puppet-katello but I thought my changes were safe here :D in any case I think the Service tomcat dependency is really needed, the rest should be okay. I propose to add spectests like these:

git diff spec/classes/candlepin_database_spec.rb
diff --git a/spec/classes/candlepin_database_spec.rb b/spec/classes/candlepin_database_spec.rb
index cfa0a8d..d9b4e3f 100644
--- a/spec/classes/candlepin_database_spec.rb
+++ b/spec/classes/candlepin_database_spec.rb
@@ -57,6 +57,8 @@ describe 'candlepin::database' do
 
           it {should contain_class('candlepin::database::postgresql') }
           it {should_not contain_class('candlepin::database::mysql') }
+          it {should contain_exec('cpdb').that_comes_before('Service[tomcat]') }
+          it {should contain_postgresql__server__role("#{db_user}").that_comes_before('Postgresql::Server::Database[candlepin]
 
           it do
             should contain_concat__fragment('PostgreSQL Database Configuration').

[ehelms]

I agree with the spec tests. I have not found what the ordering issue is though. The cpdb exec is not being called.

…

On Mar 25, 2017 1:39 PM, "Klaas Demter" ***@***.***> wrote: @ehelms <https://github.com/ehelms> this is a good point to use the spec tests, I've been meaning to do that in puppet-katello but I thought my changes were safe here :D in any case I think the Service tomcat dependency is really needed, the rest should be okay. I propose to add spectests like these: git diff spec/classes/candlepin_database_spec.rb diff --git a/spec/classes/candlepin_database_spec.rb b/spec/classes/candlepin_database_spec.rb index cfa0a8d..d9b4e3f 100644 --- a/spec/classes/candlepin_database_spec.rb +++ b/spec/classes/candlepin_database_spec.rb @@ -57,6 +57,8 @@ describe 'candlepin::database' do it {should contain_class('candlepin::database::postgresql') } it {should_not contain_class('candlepin::database::mysql') } + it {should contain_exec('cpdb').that_comes_before('Service[tomcat]') } + it {should contain_postgresql__server__role("#{db_user}").that_comes_before('Postgresql::Server::Database[candlepin] it do should contain_concat__fragment('PostgreSQL Database Configuration'). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#65 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAR58zE0cu9oBFokmgUioRIOvsXMcpFNks5rpVFngaJpZM4MowWr> .

[ehelms]

I need to fix the tests, but turns out the culprit was the refreshonly. Previously this had a notiify via a dependency chain that would trigger that refresonly as far as I can tell. Removing that with my other updates got things passing,

[Klaas-]

the tests are failing because you inserted the == "true" - thats redundant I'd say but if you want to make it explicit I think you need to use == true. I didn't remove anything as far as notifications, maybe thats from another change. The refreshonly shouldn't be needed if you use creates, the question is more like do we want cpdb to run more than once - for example if package candlepin upgrades do we want to rerun that migration?

[Klaas-]

correct that, you already fixed the == true statements :)
this lint error is because you need to clean some whitespaces in the lines: https://github.com/ehelms/puppet-candlepin/blob/89d28d4a676cdc4d45b3e97026530b369afeb1b4/manifests/database/postgresql.pp#L45-L58
when you remove the refreshonly the indentation for the => changes

[ehelms]

Updated to fix the linting issues. @Klaas- thats a good idea, given a new Candlepin package might have new migrations worth runninng. That would involve putting a notify on the cpdb for Package[candlepin] ?

[ekohl]

The travis file is modulesynced so that's going to te overridden. Other than that it looks correct.

[ehelms]

I reverted that travis change. I thought for some crazy reason it was a puppet version issue. Given 3.8 is the current latest version of the 3 line I wonder if our tests should be using that? Or if it matters?

[ekohl]

Since we have ~> 3.5 we do use 3.8. It's just not very obvious.

[ekohl]

Given this bug is closed per 0.9 and katello 3.3 includes 3.3, can we go back to cpdb?

[ehelms]

Possibly, we could file an issue to do that in a separate PR when we've got the time.

[Klaas-]

sorry for the double post - I put it into the wrong PR first :)

@ehelms I looked at the code again with a fresh pair of eyes this morning. The change I made changed a ~> to -> and that messed up the refreshonly - sorry.

In regard to changing migrations to run when an update of candlepin is noticed: this only makes sense if you actually upgrade through puppet (ie not using yum update manually). I'm not sure how this would handle inside kafo. Currently katello updates are done (following the docs) via yum update. That would mean that during the puppet run the package does not change. Keeping that in mind I'd say the migrations should rather be inside the package post scriptlets or something like that. We should just remove the refreshonly and use the creates statement - that ensures its only run once and finishes with exit 0. If it fails the "&& touch /var/lib/candlepin/cpdb_done" should not be executed and its rerun on next puppet run until it succeeds.

[ekohl]

@Klaas- I might have been the one who suggested that.

Regarding the upgrade I agree. The way the installer upgrade could force a cpdb run is simply by removing /var/lib/candlepin/cpdb_done before running puppet.