-
Notifications
You must be signed in to change notification settings - Fork 58
Fixes #16869 - only run post upgrade steps once #408
Conversation
@@ -1,3 +1,7 @@ | |||
require 'fileutils' | |||
|
|||
STEP_DIRECTORY='/etc/foreman-installer/hooks/post/' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a directory out of @kafo.config.app
? Just in case we stop using this dir.
Or, what do you think about having a hidden directory/file like we do for applied migrations? Maybe right inside the hook directory itself, we could have a .ran file or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's fine since you do mkpath, but I think I'd prefer if it was a more obvious name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see anything in @kafo.config.app that was useful. Just references to:
/etc/foreman-installer/scenarios.d/katello-answers.yaml
and
/etc/foreman-installer/scenarios.d
i could parse /etc/foreman-installer/scenarios.d and go up one or something, but that might be just as risky as hardcoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine then, just making it more obvious what it is should be good enough. e.g., applied_hooks
like we talked about on IRC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -83,7 +104,7 @@ def fail_and_exit(message) | |||
upgrade_step :restart_services |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this an always run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops! good catch
One more thought, can we add an installer option to run everything? I'd guess that'd be easier to tell a user, in case we need them to run the steps again |
👍 to the general idea, this process could use some standardization similar to migrations but i think having a solution for current makes sense. |
Added option to force --force-upgrade-steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, ACK
…ello#408) Adds one organization-validation test. Added organization-specification test. Creates test for repository search options creator. Fixes issue with filter. Fixed test issue.
No description provided.