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

Fixes #28943 - pre-upgrade check to validate external DB version #322

Merged

Conversation

kgaikwad
Copy link
Member

@kgaikwad kgaikwad commented Mar 4, 2020

No description provided.

@theforeman-bot
Copy link
Member

Issues: #28943

1 similar comment
@theforeman-bot
Copy link
Member

Issues: #28943

@theforeman-bot
Copy link
Member

Issues: #28943

@kgaikwad kgaikwad force-pushed the 28943_preupgrade_check_for_external_pg branch from 189c2b0 to 0262853 Compare March 4, 2020 11:51
def db_upgrade_message(db_version)
"\n\n*** WARNING: Server is running on PostgreSQL #{db_version}.\n"\
"*** Before performing the upgrade, please upgrade your database to PostgreSQL (>=12)\n"\
"*** otherwise data will be lost.\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean we still support old pgsql and repercussion is data loss only ? If we are not going to support the old version then message should be modified.

Copy link
Member Author

Choose a reason for hiding this comment

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

@upadhyeammit, @ehelms, any suggestion on this warning message?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, this is not a skill-able step. So I think it's ok for our language to be a bit stronger. The user must upgrade to Postgres 12 before continuing the upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-phrased the warning message. Adding console output for reference:

================================================================================
Make sure server is running on required database version:             [FAIL]


*** WARNING: Server is running on PostgreSQL 9.2.24 DB.
*** Targetted Satellite does not support PostgreSQL versions less than 12.
*** Before proceeding further, make sure you must upgrade database to PostgreSQL (>=12).

--------------------------------------------------------------------------------

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding console output for reference:

================================================================================
Make sure server is running on required database version:             [FAIL]


*** WARNING: Server is running on PostgreSQL 9.2.24 DB.
*** Newer version of Satellite supports only PostgreSQL version 12 or above.
*** Before proceeding further, make sure you must upgrade database to PostgreSQL (>=12).

--------------------------------------------------------------------------------

def db_upgrade_message(db_version)
product_name = feature(:instance).product_name

"\n\n*** WARNING: Server is running on PostgreSQL #{db_version} DB.\n"\
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should be ERROR since warnings tend to indicate something that a user should heed but is not fatal.

@kgaikwad kgaikwad force-pushed the 28943_preupgrade_check_for_external_pg branch from 7384990 to 6cb2b89 Compare March 19, 2020 14:06

"\n\n*** ERROR: Server is running on PostgreSQL #{db_version} DB.\n"\
"*** Newer version of #{product_name} supports only PostgreSQL version 12 or above.\n"\
"*** Before proceeding further, make sure you must upgrade database to PostgreSQL (>=12).\n"
Copy link
Member

Choose a reason for hiding this comment

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

s/make sure you must/you must upgrade your database/

product_name = feature(:instance).product_name

"\n\n*** ERROR: Server is running on PostgreSQL #{db_version} DB.\n"\
"*** Newer version of #{product_name} supports only PostgreSQL version 12 or above.\n"\
Copy link
Member

Choose a reason for hiding this comment

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

Sorry I didn't notice this before. There is no above in this scenario, there is only support for PostgreSQL version 12.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed! Example output:


================================================================================
Make sure server is running on required database version:             [FAIL]


*** ERROR: Server is running on PostgreSQL 9.2.24 database.
*** Newer version of Satellite supports only PostgreSQL version 12.
*** Before proceeding further, you must upgrade database to PostgreSQL 12.

@kgaikwad kgaikwad force-pushed the 28943_preupgrade_check_for_external_pg branch from 6cb2b89 to bb9d2e6 Compare March 24, 2020 19:16
@kgaikwad kgaikwad force-pushed the 28943_preupgrade_check_for_external_pg branch from bb9d2e6 to 3f9d18a Compare March 24, 2020 19:20
@kgaikwad
Copy link
Member Author

Thank you @ehelms and @upadhyeammit.
Merging this PR..

@kgaikwad kgaikwad merged commit 2d54565 into theforeman:master Mar 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants