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

Handle cases where the database connection does not use a password #763

Merged
merged 1 commit into from
Oct 24, 2024

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Oct 24, 2024

Introduced in #757

@bdunne bdunne added the bug label Oct 24, 2024
users_and_passwords = ActiveRecord::Base.connection.execute <<-SQL
SELECT rolname, rolpassword FROM pg_authid WHERE rolcanlogin;
SQL
expect(ActiveRecord::Base.connection_db_config).to receive(:configuration_hash).exactly(10).times.and_call_original
Copy link
Member Author

@bdunne bdunne Oct 24, 2024

Choose a reason for hiding this comment

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

This was an interesting find. It feels a little fragile to me, anyone have a better idea?

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 love adding "real" code only to support tests, but maybe worth it in this case. You could add a method that returns the connection_db_config in the migration class and stub that method in this spec

Copy link
Member

Choose a reason for hiding this comment

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

Why exactly 10 times as opposed to something like at least once?

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing because on the 11th time he needs to return a different payload

@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2024

@miq-bot cross-repo-test manageiq, manageiq-ui-classic, manageiq-api

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Oct 24, 2024
@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2024

I'm going to merge this to unstick the rest of the repositories but I think it's worth looking into a better way to test this.

@Fryguy Fryguy merged commit 6c25437 into ManageIQ:master Oct 24, 2024
5 checks passed
@Fryguy
Copy link
Member

Fryguy commented Oct 24, 2024

Backported to radjabov in commit 7400a8a.

commit 7400a8abb8e617b81a06945179db8dbdf2118c95
Author: Jason Frey <[email protected]>
Date:   Thu Oct 24 14:36:23 2024 -0400

    Merge pull request #763 from bdunne/no_password
    
    Handle cases where the database connection does not use a password
    
    (cherry picked from commit 6c25437ca4b62489ef801d2ab21f00da1674699c)

Fryguy added a commit that referenced this pull request Oct 24, 2024
Handle cases where the database connection does not use a password

(cherry picked from commit 6c25437)
@bdunne bdunne deleted the no_password branch October 24, 2024 18:42
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.

3 participants