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

Update Dockerfile.rhel8 to fix the postgresql upgrade issue from v12 to v13. #456

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dinaharan
Copy link

@dinaharan dinaharan commented Jul 18, 2022

Updated docker file to set the POSTGRESQL_PREV_VERSION environment variable for postgresql version 13.

Here is the OLD PR: #451

Updated docker file to set the POSTGRESQL_PREV_VERSION environment variable for postgresql version 13.
@phracek
Copy link
Member

phracek commented Jul 19, 2022

@dinaharan Why do you create a new PR? It should be enough to update the old one. Is there any reason?

@dinaharan
Copy link
Author

phracek

I noticed some difference in Dockerfile from my fork to the current and I don't want to update in the old file an that's reason created the new PR with updated Dockerfile content.

@phracek
Copy link
Member

phracek commented Jul 20, 2022

[test-all]

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

The change itself looks good to me and tests are passing.

@phracek
Copy link
Member

phracek commented Jul 21, 2022

@dinaharan As @pkubatrh mentioned the upgrade tests are not run for RHEL8 https://github.com/sclorg/postgresql-container/blob/master/test/run_test#L655. It would be nice to also fix https://github.com/sclorg/postgresql-container/blob/master/test/run_upgrade_test to support test_upgrade also for RHEL8.

Copy link
Member

@phracek phracek left a comment

Choose a reason for hiding this comment

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

This allows to upgrade into version 13. Please modify also run_upgrade_test so we have covered test suite.

Removed RHEL8 from validation so that upgrade test runs for RHEL 8.
@hhorak
Copy link
Member

hhorak commented Jul 21, 2022

[test]

@hhorak
Copy link
Member

hhorak commented Jul 21, 2022

Looking at the code, I'm worried the upgrade that is handled based on $POSTGRESQL_PREV_VERSION variable only works on RHSCL (RHEL-7), does't it? The SCL path is hardcoded in the function for example:
https://github.com/sclorg/postgresql-container/blob/705b6558/src/root/usr/share/container-scripts/postgresql/common.sh#L295

Also, I don't see we'd install postgresql-upgrade package anywhere, which would be needed for the upgrade to proceed. Or do I miss something here?

test/run_test Outdated Show resolved Hide resolved
@hhorak
Copy link
Member

hhorak commented Jul 21, 2022

Anyway, #452 that requests upgrade to work is a valid request, we just need to do more than what this PR currently changes. At minimum, install postgresql-upgrade package and also change the common.sh to work with non-scl packages.

@phracek
Copy link
Member

phracek commented Sep 14, 2022

[test-all]

@phracek
Copy link
Member

phracek commented Aug 28, 2023

@dinaharan Can you please provide updated #456 (comment). Otherwise we are not able to merge this pull request. Also please rebase this pull request agains master. THank you.

Copy link

github-actions bot commented Nov 7, 2024

Pull Request validation

Failed

🔴 Review - Missing review from a member (2 required)

Success

🟢 CI - All checks have passed
🟢 Review - Reviewed by undefined

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

5 participants