-
Notifications
You must be signed in to change notification settings - Fork 257
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
Rewrite upgrade instructions #1788
Conversation
@ehelms in theforeman/foreman-installer#646 you had much more complex instructions but I found these to work for me. Could review? |
c6118ac
to
a5523a8
Compare
I think I addressed the comments. They're in separate commits for easier review. |
I'll need to update this to remove the PG upgrade instructions since we're not going to push that into 2.4. |
And probably also incorporate #1194 which has been open for way too long. |
_includes/manuals/2.4/3.6_upgrade.md
Outdated
*Please note* that there can be significant differences between stable and | ||
development branches. You MUST take care to select a branch and make sure you | ||
get the correct one. | ||
Foreman runs best on PostgreSQL 12 but the default version on EL8 is 10. The installer will try to use version 12 but can't upgrade existing databases. An upgrade can be performed by [switching module streams](https://docs.fedoraproject.org/en-US/modularity/using-modules-switching-streams/). |
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 think you decided this no longer holds true (the part about the installer trying to use PG 12). And this upgrade to PG 12 for Foreman 2.4 is an optional step (that we should encourage users to under take).
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.
Small nit around the PG upgrade wording, everything else looks good
This limits the manual to RPM/deb packages since that's what most users use. Those who use git or tarballs are expected to handle it themselves.
Previously the menu was: 3. Installing Foreman ... 3.6 Upgrade Upgrading to Foreman 2.4 After this change it's 3. Installing Foreman ... 3.6 Upgrade to 2.4
This copies the OS selector from the quickstart. This makes the upgrade instructions shorter since you don't need to skip over irrelevant OSes. For users this is less confusing. It also removes the foreman-maintain reference. This can be done by moving the cleanup step to a point where the services should be stopped anyway. Avoiding a start-stop cycle makes the overall process shorter. The noop command removes the redundant --dont-save-answers. --noop already implies that.
Includes a workaround for BZ 1935301 in upgrades. postgresql-upgrade is unable to deal with data_directory in postgresql.conf. This is added by puppetlabs/postgresql so pretty much guaranteed to be present.
f2fac59
to
6998b96
Compare
I've updated this slightly. All changes are in the last commit (PG upgrade instructions on EL8). It now only recommends to upgrade. Please review and merge when approved. |
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.
@tbrisker final review and merge is left to you
<div class="upgrade_os upgrade_os_debian10 upgrade_os_ubuntu1804"> | ||
Upgrading from the last release to {{page.version}} has been tested. Updating | ||
the packages will upgrade the application and automatically migrate the | ||
database. |
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.
do deb packages still run migrations in postinstall?
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.
foreman-rake db:migrate | ||
foreman-rake db:seed |
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.
Don't we run migrations from the installer and seeds on startup? this should be optional probably
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 want to go full in on installer only. This procedure is safe and tested for both cases.
|
||
su - postgres -c 'vacuumdb --full --dbname=foreman' | ||
|
||
##### Optional Step 3 (C) - Run foreman-installer |
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.
considering we are aiming to have the installer as the main recommended way, shouldn't this come first and not marked as optional (with a comment it isn't relevant if you aren't using the installer)
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 want to got that far. Still plenty of improvement possible but this is now clearer than it used to be (IMHO).
##### Optional Step 3 (D) - Update config files | ||
<div class="upgrade_os upgrade_os_none"> | ||
<i>*No operating system selected.*</i> | ||
</div> | ||
<div class="upgrade_os upgrade_os_el7"> | ||
In order to catch all configuration changes and manage them properly you should install and run | ||
rpmconf from the EPEL repository along with vim-enhanced (for vimdiff). | ||
|
||
{% highlight bash %} | ||
rpmconf -a --frontend=vimdiff | ||
{% endhighlight %} | ||
</div> | ||
<div class="upgrade_os upgrade_os_el8 upgrade_os_debian10 upgrade_os_ubuntu1804"> | ||
This step is irrelevant for the chosen operating system. | ||
</div> |
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.
Perhaps only show this whole section only for upgrade_os_el7?
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 want to write that complex code. If you use HTML tags, you can't use markdown. So #####
needs to be some HTML tag.
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.
Thanks for the effort @ekohl ! I agree there is more that can be done but this is already a great improvement.
This limits the manual to RPM/deb packages since that's what most users use. Those who use git or tarballs are expected to handle it themselves.
In the next commit it copies the OS selector from the quickstart. This makes the upgrade instructions shorter since you don't need to skip over irrelevant OSes. For users this is less confusing.
It also removes the foreman-maintain reference. This can be done by moving the cleanup step to a point where the services should be stopped anyway. Avoiding a start-stop cycle makes the overall process shorter. The noop command removes the redundant --dont-save-answers. --noop already implies that.
With all of this in place it's easy to add PostgreSQL upgrade instructions only on EL8. In the previous structure it would have been too confusing.