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

Replace virtualization_tab references with kubevirt_tab #5001

Merged
merged 1 commit into from
Nov 28, 2018

Conversation

AparnaKarve
Copy link
Contributor

@AparnaKarve AparnaKarve commented Nov 27, 2018

Replace virtualization_tab references with kubevirt_tab -- this was missed in 583d776

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


Before (shows the tiny and almost invisible tab) -

screen shot 2018-11-27 at 2 52 43 pm


After (no tiny/invisible tabs) -

With Virtualization disabled -

screen shot 2018-11-27 at 2 54 55 pm

With Virtualization set to Kubevirt -

screen shot 2018-11-27 at 2 55 15 pm

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

@Fryguy
Copy link
Member

Fryguy commented Nov 27, 2018

In general, I don't think that we should be using a provider specific term in the UI. Kubevirt is a specific type of virtualization provider.

(I do understand that this particular form is not pluggable, but I'm not sure this moves anything in the right direction 😕 )

@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2018

Checked commit AparnaKarve@2cfb878 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:
warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

@AparnaKarve
Copy link
Contributor Author

@Fryguy Agreed! but this patch is simply fixing a mistake and addresses a BZ
The original code was added a while back

Copy link
Contributor

@masayag masayag left a comment

Choose a reason for hiding this comment

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

Looks good to me + verified locally (including enabling / disabling) the virtualization tab at new/update provider dialogs.

@mzazrivec mzazrivec self-assigned this Nov 28, 2018
@mzazrivec mzazrivec added this to the Sprint 100 Ending Dec 3, 2018 milestone Nov 28, 2018
@mzazrivec mzazrivec merged commit faf462e into ManageIQ:master Nov 28, 2018
simaishi pushed a commit that referenced this pull request Nov 28, 2018
Replace `virtualization_tab` references with `kubevirt_tab`

(cherry picked from commit faf462e)
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit a8493d1d076932be2f5ecee34bd30c5e3b17d286
Author: Milan Zázrivec <[email protected]>
Date:   Wed Nov 28 07:58:04 2018 +0100

    Merge pull request #5001 from AparnaKarve/fix_kubevirt_refs
    
    Replace `virtualization_tab` references with `kubevirt_tab`
    
    (cherry picked from commit faf462e9d48df5f37ef3ba65eb4e6881f4292b73)

@AparnaKarve AparnaKarve deleted the fix_kubevirt_refs branch November 28, 2018 15:44
@himdel
Copy link
Contributor

himdel commented Nov 28, 2018

Thanks for fixing this! :)

Long term, we really need that logic to come from providers, but, that's long term.

@AparnaKarve
Copy link
Contributor Author

@himdel Thanks - could not agree more!

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.

7 participants