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

Container managers #connect: don't mutate argument #13719

Merged
merged 1 commit into from
Feb 16, 2017

Conversation

cben
Copy link
Contributor

@cben cben commented Jan 31, 2017

Didn't cause any bugs that I know of, but safer not to do it.
@yaacov Please review

@@ -72,6 +72,7 @@ def verify_ssl_mode
end

def connect(options = {})
options = options.dup
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're touching the values here below anyway, you can actually do:

sanitized_options = options.merge({
  :hostname => options[:hostname] || address,
  :port     => options[:port] || port,
  ...
})

@simon3z
Copy link
Contributor

simon3z commented Jan 31, 2017

@cben what about other providers?

@simon3z
Copy link
Contributor

simon3z commented Feb 1, 2017

LGTM 👍
cc @blomquisg ready for review/merge

@miq-bot assign blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned simon3z Feb 1, 2017
@cben
Copy link
Contributor Author

cben commented Feb 1, 2017

@cben what about other providers?

Openshift inherits connect from here.
No idea what other providers do, don't think there is correlation. Sampled now couple random ones and those were fine.

Travis failure is unrelated new brakeman (kbrock has a PR for that).

@simon3z
Copy link
Contributor

simon3z commented Feb 1, 2017

@cben I think this part affects the behavior of the "Validate" button in the UI when you're in the process of adding a new provider. Did you check that everything is OK there?

@cben
Copy link
Contributor Author

cben commented Feb 1, 2017

Interesting (and troubling if so), didn't, will check.

@simon3z
Copy link
Contributor

simon3z commented Feb 10, 2017

Travis failing and a pending question on @cben

@miq-bot assign cben

@miq-bot miq-bot assigned cben and unassigned blomquisg Feb 10, 2017
@miq-bot
Copy link
Member

miq-bot commented Feb 12, 2017

Checked commit cben@722ab81 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 👍

@yaacov
Copy link
Member

yaacov commented Feb 12, 2017

LGTM 👍

@cben
Copy link
Contributor Author

cben commented Feb 12, 2017

I think this part affects the behavior of the "Validate" button in the UI when you're in the process of adding a new provider. Did you check that everything is OK there?

@simon3z New provider: main [Validate] works, hawkular [Validate] works, [Add] works.
After creation: auth good, hawkular auth good, refresh works.
Edit: main [Validate] works, hawkular [Validate] works, [Save] works. Auth & refresh still work.

Travis failure was unrelated brakeman, good after rebase.

@miq-bot assign blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned cben Feb 12, 2017
@blomquisg blomquisg merged commit f98d899 into ManageIQ:master Feb 16, 2017
@blomquisg blomquisg added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 16, 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.

6 participants