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

Rename "Cloud Instance" back to "Instance" #16647

Merged
merged 3 commits into from
Dec 14, 2017

Conversation

mzazrivec
Copy link
Contributor

Human readable model name for VmCloud was renamed from Instance to Cloud Instance in #16629 Reason for this change was a failing spec in ui-classic.

The spec in ui-classic would be failing because of changes in #15445 -- since VmCloud was added into config/miq_expression.yml under base_tables tree.

I'm reverting Cloud Instance back to Instance, because:

  1. VmCloud is just an alias for ManageIQ::Providers::CloudManager::Vm. ManageIQ::Providers::CloudManager::Vm is already called Instance so we don't want to introduce inconsitencies.
  2. Regardless of the above, we don't have any Instances that are not cloud instances, do we? There's no need to introduce this confusion into our project / product.
  3. The problem with failing spec in ui-classic can be addressed by removing VmCloud from miq_expression.yml: we don't need it there, since ManageIQ::Providers::CloudManager::Vm is already there. Also, reports using VmCloud can safely use ManageIQ::Providers::CloudManager::Vm (VmCloud is just an alias, right?)

@Fryguy @dclarizio

@himdel
Copy link
Contributor

himdel commented Dec 12, 2017

LGTM, this resolves my consistency comment, thanks! :)

@dclarizio
Copy link

The problem with failing spec in ui-classic can be addressed by removing VmCloud from miq_expression.yml: we don't need it there, since ManageIQ::Providers::CloudManager::Vm is already there. Also, reports using VmCloud can safely use ManageIQ::Providers::CloudManager::Vm (VmCloud is just an alias, right?)

@mzazrivec do we know when VmCloud was added to the yaml file? Just want to make sure there aren't any expressions/reports out there that this might break.

@mzazrivec
Copy link
Contributor Author

@mzazrivec do we know when VmCloud was added to the yaml file? Just want to make sure there aren't any expressions/reports out there that this might break.

It was added in this PR: #15445

@dclarizio
Copy link

@mzazrivec do we know when VmCloud was added to the yaml file? Just want to make sure there aren't any expressions/reports out there that this might break.

It was added in this PR: #15445

So do we need to change this line as well then?

@gtanzillo could use your input as well

@mzazrivec
Copy link
Contributor Author

@dclarizio well this PR changes the line you're referring to, right? Or maybe I don't understand the question -- do you think the line you're showing should remain unmodified?

@himdel
Copy link
Contributor

himdel commented Dec 12, 2017

That line should probably refer to the actual class as well, not to the alias.

@dclarizio
Copy link

@mzazrivec indeed it does, somehow I missed that! 😄 This should be good to go when green. Thx!

VmCloud is just an alias for ManageIQ::Providers::CloudManager::Vm, it's
called Instance and we don't want to introduce inconsistencies.

Revert "Rename pretty modelname- VmCloud from 'Instance to 'Cloud Instance'"

This reverts commit 41d8008.
VmCloud is just an alias for ManageIQ::Providers::CloudManager::Vm
@mzazrivec mzazrivec force-pushed the there_is_no_cloud_instance branch from 01e60ba to 450a9be Compare December 13, 2017 10:02
@miq-bot
Copy link
Member

miq-bot commented Dec 13, 2017

Checked commits mzazrivec/manageiq@44747a6~...450a9be with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@mzazrivec
Copy link
Contributor Author

@dclarizio CI fixed.

@dclarizio dclarizio merged commit d5064ab into ManageIQ:master Dec 14, 2017
@dclarizio dclarizio added this to the Sprint 76 Ending Jan 1, 2018 milestone Dec 14, 2017
simaishi pushed a commit that referenced this pull request Dec 14, 2017
Rename "Cloud Instance" back to "Instance"
(cherry picked from commit d5064ab)
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 86fa54703cb4aa35c1cd710cd0acabea77ff499c
Author: Dan Clarizio <[email protected]>
Date:   Thu Dec 14 07:03:01 2017 -0800

    Merge pull request #16647 from mzazrivec/there_is_no_cloud_instance
    
    Rename "Cloud Instance" back to "Instance"
    (cherry picked from commit d5064ab331a6ace409cef69390e8a0b7f8d0ec54)

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.

5 participants