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

must give password to set db in cli #281

Merged
merged 2 commits into from
Oct 13, 2017

Conversation

ailisp
Copy link
Member

@ailisp ailisp commented Oct 4, 2017

ISSUE: Currently we allow user to set up database in appliance console cli without given a password, which should be disallowed. This is part of BZ https://bugzilla.redhat.com/show_bug.cgi?id=1431815
Another part (disallow --internal without --dbdisk) is fixed in #277

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1431815#c2

\cc @yrudman @carbonin @gtanzillo

@miq-bot add-label wip, bug

@ailisp ailisp force-pushed the must-give-pwd-cli branch 2 times, most recently from 863849c to d5f7d20 Compare October 5, 2017 19:43
@ailisp
Copy link
Member Author

ailisp commented Oct 5, 2017

@miq-bot remove-label wip

@miq-bot miq-bot changed the title [WIP] must give password to set db in cli must give password to set db in cli Oct 5, 2017
@miq-bot miq-bot removed the wip label Oct 5, 2017
@@ -182,13 +182,18 @@ def run

def set_db
raise "No encryption key (v2_key) present" unless key_configuration.key_exist?
raise "Must provide a password to set database" unless password?
Copy link
Member

Choose a reason for hiding this comment

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

I think we can improve this sentence a bit ...

Maybe "A password is required to configure a database"

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

def password?
options[:password] && !options[:password].strip.empty?
end

Copy link
Contributor

@yrudman yrudman Oct 13, 2017

Choose a reason for hiding this comment

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

would below look more complicated or easer to read ? :
options[:password] ? !options[:password].strip.empty? : false

Also, I would fix style warning

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine the way it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @yrudman and @carbonin . Now should be good.

@carbonin
Copy link
Member

Looks good @ailisp. Just fix up the rubocop complaints and we should be good to go.

In this case you can either just add the spaces or (as the rest of this file seems to do), use expect with the do ... end block notation. I'm fine with either.

@miq-bot
Copy link
Member

miq-bot commented Oct 13, 2017

Checked commits ailisp/manageiq-gems-pending@b748667~...e462cfc with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

@carbonin carbonin merged commit 111571b into ManageIQ:master Oct 13, 2017
@carbonin carbonin added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 13, 2017
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.

4 participants