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

Ensure that error indicator appears on Hawkular tab #1172

Merged
merged 2 commits into from
Apr 27, 2017
Merged

Ensure that error indicator appears on Hawkular tab #1172

merged 2 commits into from
Apr 27, 2017

Conversation

AparnaKarve
Copy link
Contributor

@AparnaKarve AparnaKarve commented Apr 26, 2017 •
edited
Loading

Display the Hawkular tab error indicator while adding a new Container Provider.

screen shot 2017-04-27 at 9 58 21 am

https://bugzilla.redhat.com/show_bug.cgi?id=1445735

@AparnaKarve
Copy link
Contributor Author

@moolitayer I have fixed the Hawkular tab as we discussed.

I figured that with this change, we would also need the Hawkular Hostname and Token fields to be Required which I have addressed in the second commit 531549c

If this resolves the bug, we can make identical changes for RHEV for the Metrics tab.

@AparnaKarve AparnaKarve force-pushed the bz1445735_fix_error_icon_on_tabs branch 2 times, most recently from 0f16e59 to abd1daf Compare April 26, 2017 21:21
@moolitayer
Copy link

@AparnaKarve tested and this works great. The mandatory hostname & port addition is also good.

I don't think we should add hawkular_password and hawkular_verify. I'm pretty sure it's by design that in our hawkular tab we do not display passwords and use the same one from our default endpoint.
(also the validate ignores those)

Correct me if I'm wrong @simon3z @cben @zeari

@moolitayer
Copy link

To be clear,
Before change:
before

After change:

after

@AparnaKarve
Copy link
Contributor Author

@moolitayer Thanks for testing.

I will remove the password and verify fields for Hawkular

@AparnaKarve AparnaKarve force-pushed the bz1445735_fix_error_icon_on_tabs branch from abd1daf to cac62c7 Compare April 27, 2017 16:54
@miq-bot
Copy link
Member

miq-bot commented Apr 27, 2017

Checked commits AparnaKarve/manageiq-ui-classic@e5bc475~...cac62c7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🏆

@AparnaKarve
Copy link
Contributor Author

@moolitayer I have adjusted the last commit (cac62c7) to not display the Token fields for Hawkular. The only change that I have made is Hostname and Port are now Required fields.

moolitayer
moolitayer approved these changes Apr 27, 2017
View reviewed changes
Copy link

@moolitayer moolitayer left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@dclarizio dclarizio self-assigned this Apr 27, 2017
@dclarizio dclarizio merged commit d703654 into ManageIQ:master Apr 27, 2017
@dclarizio dclarizio added this to the Sprint 60 Ending May 8, 2017 milestone Apr 27, 2017
@moolitayer
Copy link

@dclarizio this should not go to fine. We must add together with this an ability to disable metrics explicitly.
Note before this fix the endpoint was being added but was being added corrupted (e.g no hostname or no port)

BTW why is this fine/yes if the bz is targeted at 5.8.1 ?

cc @simon3z @AparnaKarve

@moolitayer
Copy link

removing label per #1172 (comment).
@dclarizio please update if there is a reason this is needed on fine
@miq-bot remove_label fine/yes
@miq-bot add_label fine/no

@miq-bot miq-bot added fine/no and removed fine/yes labels May 1, 2017
@dclarizio
Copy link

@moolitayer

BTW why is this fine/yes if the bz is targeted at 5.8.1 ?

Fine IS 5.8, so if we want it in any version of 5.8, we would need to label as fine/yes. It just won't be backported until we start building 5.8.1. If there is another fix that needs to go with this, perhaps leave it fine/no for now, then mark them both as yes when ready.

cben added a commit to cben/manageiq-ui-classic that referenced this pull request May 8, 2017
@cben
Containers provider Edit: handle hawkular port being null.
88dc9ef 
It was possible to add such provider prior to ManageIQ#1172.
This makes Edit not crash but display port 443 (and save it upon Save).

https://bugzilla.redhat.com/show_bug.cgi?id=1432070
@cben cben mentioned this pull request May 8, 2017
Closed
@moolitayer
Copy link

Thanks for the explanation @dclarizio
@miq-bot add_label fine/yes

All pieces should be ready by next release

@moolitayer
Copy link

@miq-bot remove_label fine/no

@miq-bot miq-bot removed the fine/no label May 9, 2017
@simaishi
Copy link
Contributor

simaishi commented Jun 5, 2017 •
edited
Loading

@moolitayer which PR needs to go together with this PR for Fine branch? Marking as fine/conflict for now.

@moolitayer
Copy link

Thanks @simaishi I've responded on https://bugzilla.redhat.com/show_bug.cgi?id=1437138 . We either include all fixes or none in 5.8.1

@moolitayer
Copy link

@miq-bot add_label fine/no

cben added a commit to cben/manageiq-ui-classic that referenced this pull request Jun 19, 2017
@cben
Backport of ManageIQ#1305
a1be655 
It is possible to add such provider on fine (fixed in ManageIQ#1172, fine/no).
This makes Edit not crash but display port 443 (and save it upon Save).

https://bugzilla.redhat.com/show_bug.cgi?id=1432070
@cben cben mentioned this pull request Jun 19, 2017
Merged
cben added a commit to cben/manageiq-schema that referenced this pull request Oct 18, 2017
@cben
Convert invalid containers hawkular Endpoints port=nil to port=0
cb07dca 
For a time (before ManageIQ/manageiq-ui-classic#1172),
it was possible in UI to save containers provider without hawkular port
and then it'd be impossible to edit the provider as UI would crash.

This migration normalizes such endpoints to port 0, still invalid but
possible to edit in UI, so UI won't need a nil special case.

https://bugzilla.redhat.com/show_bug.cgi?id=1432070
@cben cben mentioned this pull request Oct 18, 2017
Merged
cben added a commit to cben/manageiq-schema that referenced this pull request Oct 30, 2017
@cben
Convert invalid containers hawkular Endpoints port=nil to port=443
12df92c 
For a time (before ManageIQ/manageiq-ui-classic#1172),
it was possible in UI to save containers provider without hawkular port
and then it'd be impossible to edit the provider as UI would crash.

AFAICT, such providers were effectively using port 443
(builds hawkular url https://<host>/ without port, HTTPS defaults to 443).
This migration normalizes such endpoints to port 443.

This makes it possible to edit in UI, without adding a nil special case.
We'll also be able to simplify hawkular connection code in future.

https://bugzilla.redhat.com/show_bug.cgi?id=1432070
cben added a commit to cben/manageiq-schema that referenced this pull request Oct 30, 2017
@cben
Convert invalid containers hawkular Endpoints port=nil to port=443
dd5ed57 
For a time (before ManageIQ/manageiq-ui-classic#1172),
it was possible in UI to save containers provider without hawkular port
and then it'd be impossible to edit the provider as UI would crash.

AFAICT, such providers were effectively using port 443
(builds hawkular url https://<host>/ without port, HTTPS defaults to 443).
This migration normalizes such endpoints to port 443.

This makes it possible to edit in UI, without adding a nil special case.
We'll also be able to simplify hawkular connection code in future.

https://bugzilla.redhat.com/show_bug.cgi?id=1432070
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moolitayer moolitayer moolitayer approved these changes

Assignees

@dclarizio dclarizio

Labels
bug middleware
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AparnaKarve @moolitayer @miq-bot @dclarizio @simaishi