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

Fix library/helpers.rb pulling different package names with multiple PG installations #765

Merged
merged 5 commits into from
Jan 24, 2024

Conversation

Justin-Fernbaugh
Copy link
Contributor

Description

Describe what this change achieves

Currently with multiple PG installations "installed_postgresql_major_version" and "installed_postgresql_package_source" have a chance to pull a different package from each other. The fix is to sort the packages after being filtered such that calling ".first" always returns the same value.

Issues Resolved

The functions "installed_postgresql_major_version" and "installed_postgresql_package_source" will maintain the same value.

Check List

  • A summary of changes made is included in the CHANGELOG under ## Unreleased
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@Justin-Fernbaugh Justin-Fernbaugh requested a review from a team as a code owner December 14, 2023 20:03
@ramereth ramereth added the Release: Minor Release to Chef Supermarket as a minor release when merged label Dec 14, 2023
@@ -25,6 +25,7 @@ module Helpers

def installed_postgresql_major_version
pgsql_package = node['packages'].filter { |p| p.match?(/^postgresql-?(\d+)?$/) }
pgsql_package = pgsql_package.sort_by { |key, _values| key[1].to_i }.to_h
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to work properly and I'm confused why you're converting that value to an int. If I just change it to the following, it seems to work properly.

Suggested change
pgsql_package = pgsql_package.sort_by { |key, _values| key[1].to_i }.to_h
pgsql_package = pgsql_package.sort_by { |key, _values| key }.to_h

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @ramereth here, it doesn't work nor make sense from what I can work out. I'd sort against the version key if we wanted to always order by version but that doesn't necessarily help us out.

I'm thinking the better option would be to move the package match logic into a seperate method which installed_postgresql_major_version and installed_postgresql_package_source could use to get the package so they'll always tie up.

Further to that, I think that matching more than one package is an unknown that we can't handle 100% so it might be better to raise if we get more than one package matched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Really appreciate the suggestions on how to improve this. @bmhughes Moving the matching logic into a function I agree makes a lot of sense here. With that said, the goal of sorting was to select for the newest PG version in order to somewhat accommodate multiple PG installations.

What are the thoughts on implementing a property overriding the match found from either installed_postgresql_major_version or installed_postgresql_package_source . This way those who have more than one PG version on a machine can force one to be used. Also, I'm suggesting to implement this so that upgrades can be performed without having to write custom resource. Thanks for the feedback

Copy link
Contributor

Choose a reason for hiding this comment

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

@bmhughes As @Justin-Fernbaugh mentioned, this logic is only needed during an in-place upgrade when you will have multiple versions installed. This provides a stable fix so that we can safely do this. This isn't intended to be used long term on a node.

@bmhughes bmhughes removed the Release: Minor Release to Chef Supermarket as a minor release when merged label Dec 15, 2023
@ramereth
Copy link
Contributor

@Justin-Fernbaugh can you please update and rebase?

@Justin-Fernbaugh Justin-Fernbaugh force-pushed the fernbauj/multi-db-support branch from da7f2f5 to 47c92c9 Compare December 20, 2023 17:16
@Justin-Fernbaugh
Copy link
Contributor Author

@Justin-Fernbaugh can you please update and rebase?

Just pulled and rebased

@Justin-Fernbaugh Justin-Fernbaugh force-pushed the fernbauj/multi-db-support branch 3 times, most recently from ca32664 to 62ef806 Compare December 20, 2023 18:32
ramereth
ramereth previously approved these changes Dec 20, 2023
Copy link
Contributor

@ramereth ramereth left a comment

Choose a reason for hiding this comment

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

@bmhughes let me know if you have any other concerns and what kind of a bump you'd like

@ramereth ramereth force-pushed the fernbauj/multi-db-support branch from 35c1c73 to cdacedd Compare January 5, 2024 22:07
@ramereth ramereth added the Release: Minor Release to Chef Supermarket as a minor release when merged label Jan 5, 2024
@ramereth ramereth requested a review from bmhughes January 5, 2024 22:08
Reverse hashmap of pg_version helpers

Style correction modify variable name

Remove commented code

Signed-off-by: Justin Fernbaugh <fernbaughj@gmail.com>

Revert minor bump

Signed-off-by: Justin Fernbaugh <fernbaughj@gmail.com>

Sort array with highest PG version first

Add changelog note

Signed-off-by: Justin Fernbaugh <fernbaughj@gmail.com>

Cleanup sort_by

Move package logic into function

Removed changelog addition

Modify sort logic to select based on version float
@Justin-Fernbaugh Justin-Fernbaugh force-pushed the fernbauj/multi-db-support branch 4 times, most recently from 1b34793 to ea65280 Compare January 22, 2024 19:14
Remove blank line from ci.yml

Remove blank lines, add blank line at EOF

Remove blank line from kitchen.yml

Fix removed character

Remove trailing space in ci.yml
@Justin-Fernbaugh Justin-Fernbaugh force-pushed the fernbauj/multi-db-support branch from ea65280 to 787edbc Compare January 22, 2024 19:17
CHANGELOG.md Outdated Show resolved Hide resolved
Combine two lines of documentation
@Justin-Fernbaugh Justin-Fernbaugh force-pushed the fernbauj/multi-db-support branch from 5eb355c to fac86c2 Compare January 22, 2024 19:51
ramereth
ramereth previously approved these changes Jan 22, 2024
@bmhughes
Copy link
Contributor

Sorry I've completely missed this pinging away, let me re-review and try some things out.

libraries/helpers.rb Show resolved Hide resolved
@ramereth ramereth requested a review from bmhughes January 23, 2024 22:24
@bmhughes
Copy link
Contributor

I'm going to merge this but on the whole I'm not 100% sure this is the correct way to go, I thinking it'd be better to raise on multiple versions detected and force the user to explicitly specify the version they want to work on rather than have an implicit behaviour that isn't necessarily deterministic.

But let's see how it goes.

@bmhughes bmhughes merged commit ab478ef into sous-chefs:main Jan 24, 2024
228 checks passed
@kitchen-porter
Copy link
Contributor

Released as: 11.10.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release: Minor Release to Chef Supermarket as a minor release when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants