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

Fix protocol selection when adding a new nuage provider #45

Merged
merged 1 commit into from
Dec 13, 2017
Merged

Fix protocol selection when adding a new nuage provider #45

merged 1 commit into from
Dec 13, 2017

Conversation

gasper-vrhovsek
Copy link
Contributor

This fixes endpoint url generation from form input values. Before, if SSL was selected, the url began
with 'http'. Now when either 'SSL' or 'SSL with validation' are selected, the url begins with 'https'.

@miq-bot add_label bug

/cc @gberginc
/cc @miha-plesko

@miq-bot miq-bot added the bug label Nov 20, 2017
@gberginc gberginc self-requested a review November 20, 2017 16:37
@gberginc
Copy link
Contributor

@gasper-vrhovsek please fix rubocop issues.

@miq-bot add_label gaprindashvili/yes

@gasper-vrhovsek gasper-vrhovsek force-pushed the feature/fix_protocol_selection branch from 4ee4082 to 6a3413b Compare November 21, 2017 07:42
miha-plesko
miha-plesko suggested changes Nov 22, 2017
View reviewed changes
Copy link
Contributor

@miha-plesko miha-plesko left a comment

Choose a reason for hiding this comment

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

@gasper-vrhovsek nitpicking, but I'd simplify the ternary operator a little bit more.

app/models/manageiq/providers/nuage/manager_mixin.rb Outdated
@@ -17,7 +17,7 @@ def raw_connect(username, password, endpoint_opts)
end

def auth_url(protocol, server, port, version)
scheme = protocol == "ssl-with-validation" ? "https" : "http"
scheme = !protocol || (protocol == "non-ssl") ? "http" : "https"
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this !protocol part confusing*, perhaps we would be better off with:

scheme = protocol.to_s.start_with? "ssl" ? "https" : "http"

*Too much Java-like. I mean, we're in ruby, we shouldn't check for null all over the code 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean nil ? :)

@gasper-vrhovsek gasper-vrhovsek force-pushed the feature/fix_protocol_selection branch 2 times, most recently from 1ba9c1b to 8e63b1c Compare December 4, 2017 12:47
@gasper-vrhovsek
Copy link
Contributor Author

gasper-vrhovsek commented Dec 4, 2017 •
edited
Loading

@miha-plesko I have reverted to the !protocol solution, as otherwise this test fails:

https://github.com/ManageIQ/manageiq-providers-nuage/blob/master/spec/models/manageiq/providers/nuage/network_manager_spec.rb#L144

bronaghs
bronaghs reviewed Dec 6, 2017
View reviewed changes
app/models/manageiq/providers/nuage/manager_mixin.rb Outdated
@@ -17,7 +17,7 @@ def raw_connect(username, password, endpoint_opts)
end

def auth_url(protocol, server, port, version)
scheme = protocol == "ssl-with-validation" ? "https" : "http"
scheme = (!protocol || protocol == "non-ssl") ? "http" : "https"
Copy link

Choose a reason for hiding this comment

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

@gasper-vrhovsek
This is difficult to read. Can you describe the logic here so I can make sure I understand it correctly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bronaghs, sure i can describe:

If protocol is not provided or if 'non-ssl' is selected, then 'http' value is assigned. Otherwise 'https' is assigned.

My background is mostly java, so I am still getting used to Ruby. I'll search for a more readable solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bronaghs and @miha-plesko I have refactored the scheme selecting logic. Hope it's better and more readable now.

@gasper-vrhovsek gasper-vrhovsek force-pushed the feature/fix_protocol_selection branch 4 times, most recently from 7ee6e07 to ca40ad5 Compare December 7, 2017 10:34
@gasper-vrhovsek gasper-vrhovsek force-pushed the feature/fix_protocol_selection branch from ca40ad5 to ac9f881 Compare December 7, 2017 10:43
@miq-bot
Copy link
Member

miq-bot commented Dec 7, 2017

Checked commit gasper-vrhovsek@ac9f881 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪

@bronaghs
Copy link

bronaghs commented Dec 7, 2017

Thanks @gasper-vrhovsek for reworking it 👍

@juliancheal juliancheal self-assigned this Dec 13, 2017
@juliancheal juliancheal added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 13, 2017
@juliancheal juliancheal merged commit 3887e05 into ManageIQ:master Dec 13, 2017
@gasper-vrhovsek gasper-vrhovsek deleted the feature/fix_protocol_selection branch December 13, 2017 10:21
simaishi pushed a commit that referenced this pull request Dec 14, 2017
@juliancheal @simaishi
Merge pull request #45 from gasper-vrhovsek/feature/fix_protocol_sele…
935da22 
…ction

Fix protocol selection when adding a new nuage provider
(cherry picked from commit 3887e05)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 935da2208632d5ae3f78106c0a4daf96213686dc
Author: Julian Cheal <[email protected]>
Date:   Wed Dec 13 09:57:10 2017 +0000

    Merge pull request #45 from gasper-vrhovsek/feature/fix_protocol_selection
    
    Fix protocol selection when adding a new nuage provider
    (cherry picked from commit 3887e05bdda08c68c784450abe524e99a2f1bbe0)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miha-plesko miha-plesko miha-plesko requested changes

@bronaghs bronaghs bronaghs approved these changes

@gberginc gberginc Awaiting requested review from gberginc

Assignees

@juliancheal juliancheal

Labels
bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gasper-vrhovsek @gberginc @miq-bot @bronaghs @simaishi @miha-plesko @juliancheal