Fixes #37717 - Update evr extension ownership to foreman

[sjha4]

Update ownership of evr extension to owner foreman to allow migrations to operate on it, specifically disable it.

[ianballou]

@ekohl mentioned before that the pg_shdepend table might also need updating, but at least functionally it doesn't seem necessary since the migration passes with @sjha4 's changes alone. The bits that would get updated in pg_shdepend may get dropped with the evr extension.

[ianballou]

I confirmed that dropping the extension also drops the related entry from pg_shdepend.

[sjha4]

I tested the installer with the chages here although I had to put the new code in a separate pre-hook file to be run..Suggestions on where to put this code if not this file?

[evgeni]

What about remote db users? Will those need to perform manual steps (we usually don't have enough permissions to change ownership of objects there) before the upgrade?

[sjha4]

What about remote db users? Will those need to perform manual steps (we usually don't have enough permissions to change ownership of objects there) before the upgrade?

Ya..They'll need to run the update ownership query manually as part of their upgrades. Will need to be documented.

[ianballou]

@evgeni docs update will go in theforeman/foreman-documentation#3167 unless someone gets to it first.

[ianballou]

Prepping a system for a test.

[ianballou]

Found an issue -- the postgres command cannot run because the postgres service is down during the installer run. We'll need to turn postgres on first.

[ekohl]

I'm not sure about the noop mode implementation here. I'd be tempted to change the log messages to Would do X instead of Doing x`.

[ehelms]

LGTM -- I'll let @ekohl have the final say.

[ekohl]

Style wise I'm not that happy with the many conditionals and prefer the more verbose:

if app_value(:noop)
  logger.debug('Would do X')
else
  logger.debug('Doing X')
  x()
end

My reasoning is that it's very easy to visually see the control flow.

Additionally, we already have debug statements in various methods, like start_services and stop_services. So you can simplify it to:

if app_value(:noop)
  logger.debug('Would stop service postgresql')
else
  stop_services(['postgresql'])
end

I'm wondering what you think about this.

[ekohl]

Thanks for updating. I think this is much easier to reason about and see where it could fail.

[ekohl]

I like how this is shaping up now. Some minor style comments.

[ekohl]

@evgeni I made may suggestions so I'd appreciate if you gave it a final ACK.

[evgeni]

I think failing to update should be a warning?

[ekohl]

Warning or even error that halts execution?

[evgeni]

Maybe. It was a warning before, so that's what caught my eye.

What happens if it is not updated? Later the removal of the extension fails?

[sjha4]

Ya..The evr extension is dropped in katello migration here Katello/katello#11159 which assumes the foreman user is the owner of the extension. We could fail here instead of later on during db:migrate.

[sjha4]

Updated to fail and exit:

fail_and_exit("Failed to update ownership of the evr extension. Please make sure the following sql succeeds before proceeding: #{sql}")

[sjha4]

@evgeni @ekohl Is this what y'all had in mind?

[sjha4]

I will need someone with superpowers to merge this one if this is good to go..
Thanks @ekohl , @ehelms , @evgeni and @ianballou ❤️

[ekohl]

Thanks!