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.

Sign up for GitHub

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

Jump to bottom

Don't set empty ENV values for database dumps #378

Merged
merged 1 commit into from
Oct 8, 2018
Merged

Don't set empty ENV values for database dumps #378

merged 1 commit into from
Oct 8, 2018

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Oct 4, 2018

Currently, PGUSER and PGPASSWORD are set in the environment.
We want these values to pass through to the subshells

Before

$PGUSER was overridden by "" if --dbuser was not passed into the rake task

After

The existing $PGUSER value is used if there is no --dbuser passed in, but the new value is used if one is passed in.
(same goes for $PGPASSWORD)

related to:

@NickLaMuro
Copy link
Member

@kbrock I am fine with this PR, but I am questioning the bug label.

I have backups/dumps working on every provider we have fully implemented in an automated test suite without this change, so I am curious what is different with your setup that is making this necessary for things to work.

carbonin
carbonin reviewed Oct 5, 2018
View reviewed changes
lib/gems/pending/util/postgres_admin.rb Outdated
@@ -249,7 +249,7 @@ def self.runcmd_with_logging(cmd_str, opts, params = {})
{
"PGUSER" => opts[:username],
"PGPASSWORD" => opts[:password]
}
}.delete_if { |n, v| v.blank? }
Copy link
Member

Choose a reason for hiding this comment

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

Do we have delete_blanks here? I can't remember if that's activesupport or morecore ...

Also, can you fix the code climate/bot comment about the unused argument?

Copy link
Member Author

@kbrock kbrock Oct 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

more_core_extensions is in the gemspec, so it should be good. (I always avoid morecore in gems but in this case, it is good. thnx)

@kbrock kbrock added enhancement and removed bug labels Oct 6, 2018
@kbrock
Copy link
Member Author

kbrock commented Oct 6, 2018 •
edited
Loading

@NickLaMuro I always thought you put BUG when you had a BZ. even if the BZ is an RFE. I changed

@miq-bot
Copy link
Member

miq-bot commented Oct 6, 2018

Checked commit kbrock@2d4f718 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@carbonin carbonin self-assigned this Oct 8, 2018
@carbonin carbonin merged commit 3f6a759 into ManageIQ:master Oct 8, 2018
@carbonin carbonin added this to the Sprint 96 Ending Oct 8, 2018 milestone Oct 8, 2018
simaishi pushed a commit that referenced this pull request Oct 8, 2018
@carbonin @simaishi
Merge pull request #378 from kbrock/env_fixup
9a5d8a0 
Don't set empty ENV values for database dumps

(cherry picked from commit 3f6a759)
@simaishi
Copy link
Contributor

simaishi commented Oct 8, 2018

Hammer backport details:

$ git log -1
commit 9a5d8a05a209b25531fa042db96dc03dda8be568
Author: Nick Carboni <[email protected]>
Date:   Mon Oct 8 09:07:03 2018 -0400

    Merge pull request #378 from kbrock/env_fixup
    
    Don't set empty ENV values for database dumps
    
    (cherry picked from commit 3f6a759e18cab9e162e88792bbe3977d64180596)

@kbrock kbrock deleted the env_fixup branch October 9, 2018 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbonin carbonin carbonin approved these changes

Assignees

@carbonin carbonin

Labels
enhancement hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kbrock @NickLaMuro @miq-bot @simaishi @carbonin