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

Move security protocol #759

Merged
merged 2 commits into from Mar 27, 2017
Merged

Conversation

josejulio
Copy link
Member

@josejulio josejulio commented Mar 21, 2017

As pointed by @abonas on #460 (comment) it would be better to have security protocol before port.
@Loicavenel commented that we should at least reverse port and security protocol (#460 (comment)).
@cben proposed to mimic the URI structure (protocol://hostname:port) (#460 (comment)) which is included on this PR.

Since this fields are shared among other providers I'm doing this change in a separate PR expecting other provider maintainers who use these two fields would comment / review that it indeed won't break anything for them or at least notify them of this change.

If needed I could move security protocol before port.

Additionally I'm fixing ham-lint warnings on that file.

Here are some screenshots of providers that i saw are using the Security Protocol.

Containers provider


screenshot from 2017-03-21 13-34-39


screenshot from 2017-03-21 13-34-34

Infrastructure provider


screenshot from 2017-03-21 13-33-42


screenshot from 2017-03-21 13-33-36


screenshot from 2017-03-23 13-07-55

Cloud provider


screenshot from 2017-03-21 13-32-47

Middleware provider


screenshot from 2017-03-21 13-14-24

@cben could you please take a look?

@josejulio
Copy link
Member Author

@miq-bot add_label euwe/no

@josejulio
Copy link
Member Author

Know other provider maintainers who could take a look at this?

@cben
Copy link
Contributor

cben commented Mar 22, 2017

Thanks for thorough screenshots :-) LGTM 👍.

cc @Ladas @blomquisg

@cben
Copy link
Contributor

cben commented Mar 22, 2017

@miq-bot add-labels compute/infrastructure, middleware

@miq-bot
Copy link
Member

miq-bot commented Mar 23, 2017

This pull request is not mergeable. Please rebase and repush.

@abonas
Copy link
Member

abonas commented Mar 23, 2017

@miq-bot assign @israel-hdez
@israel-hdez please review
fyi @oourfali

@miq-bot miq-bot assigned israel-hdez and unassigned dclarizio Mar 23, 2017
@abonas
Copy link
Member

abonas commented Mar 23, 2017

@josejulio please rebase

@josejulio
Copy link
Member Author

@abonas rebasing seems to be a bit tricky.
Since I moved code (which translates on deleting and adding code) I need to see what changed and apply the changes on the 'added' code.
Could i get a reviewed before rebasing? If you feel that rebasing is a need for reviewing i 'll rebase now. But I guess I'll soon need to rebase again because code is shared with other providers.

@oourfali
Copy link

@jhernand can you take a look?

@Loicavenel
Copy link

@tzumainn can you into this PR to do the same for OpenStack.. protocol first then port

@josejulio
Copy link
Member Author

It turns out it wasn't that tricky thanks to this: cff36d4...6653e31#diff-f5d9ed4e8acd0a9cb7e661a8571697f1

Port value is affected by Security Protocol, when one protocol is selected
it sets the default port value (on some providers) of the protocol.
@miq-bot
Copy link
Member

miq-bot commented Mar 23, 2017

Checked commits josejulio/manageiq-ui-classic@6ee34bd~...81b93e2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@cben
Copy link
Contributor

cben commented Mar 23, 2017

@Loicavenel @tzumainn this PR already affects Openstack (see screenshots).

Copy link
Member

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

LGTM

Tried it. Works fine when adding a new middleware provider. In other affected places, the form is being displayed correctly, but cannot confirm if submitting the form still works (although I don't see anything that could break the other forms)

@jhernand
Copy link
Contributor

Looks good to me. In oVirt there is no way to select the security protocol, as it is mandatory to use TLS, so this shouldn't affect. @josejulio would be nice if you can add an screen shot showing the result of New infrastructure provider but selecting Red Hat Virtualization, just to make sure that it still looks correct.

@josejulio
Copy link
Member Author

@jhernand Sure, I just updated the screenshots to include that one.

@jhernand
Copy link
Contributor

Thanks @josejulio, that looks correct.

@abonas
Copy link
Member

abonas commented Mar 27, 2017

@josejulio very thorough, kudos!
auto filling the port based on the protocol is out of scope for this patch, correct?

@josejulio
Copy link
Member Author

Thanks!
The auto filling was done in the previous PR. It should be already working.

@abonas
Copy link
Member

abonas commented Mar 27, 2017

@AparnaKarve please review/merge, it has been acked by multiple reviewers

@AparnaKarve
Copy link
Contributor

LGTM.
@h-kataria Can you please merge this?

@h-kataria h-kataria added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017
@h-kataria h-kataria merged commit 942730d into ManageIQ:master Mar 27, 2017
@abonas
Copy link
Member

abonas commented Mar 28, 2017

@miq-bot add_label enhancement

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.

None yet