Skip to content
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

Merged
merged 5 commits into from
Mar 18, 2021

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Mar 4, 2021

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.

@ekohl
Copy link
Member Author

ekohl commented Mar 4, 2021

@ehelms in theforeman/foreman-installer#646 you had much more complex instructions but I found these to work for me. Could review?

@ekohl ekohl force-pushed the rewrite-upgrading-guide branch 2 times, most recently from c6118ac to a5523a8 Compare March 4, 2021 12:21
@ekohl
Copy link
Member Author

ekohl commented Mar 9, 2021

I think I addressed the comments. They're in separate commits for easier review.

@ekohl
Copy link
Member Author

ekohl commented Mar 10, 2021

I'll need to update this to remove the PG upgrade instructions since we're not going to push that into 2.4.

@ekohl ekohl marked this pull request as draft March 10, 2021 18:02
@ekohl
Copy link
Member Author

ekohl commented Mar 10, 2021

And probably also incorporate #1194 which has been open for way too long.

*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/).
Copy link
Member

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).

Copy link
Member

@ehelms ehelms left a 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.
@ekohl ekohl marked this pull request as ready for review March 11, 2021 15:45
@ekohl
Copy link
Member Author

ekohl commented Mar 11, 2021

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.

@ekohl ekohl requested review from tbrisker and ehelms March 11, 2021 15:46
Copy link
Member

@ehelms ehelms left a 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.
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 188 to 189
foreman-rake db:migrate
foreman-rake db:seed
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

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)

Copy link
Member Author

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).

Comment on lines +219 to +233
##### 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>
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

@tbrisker tbrisker left a 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.

@tbrisker tbrisker merged commit b2b43db into theforeman:gh-pages Mar 18, 2021
@ekohl ekohl deleted the rewrite-upgrading-guide branch March 18, 2021 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants