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

GuestDevice model updates for support of storage devices #17332

Merged
merged 2 commits into from
Apr 25, 2018

Conversation

skovic
Copy link

@skovic skovic commented Apr 23, 2018

This PR updates the GuestDevice model to support storage devices. In addition, the alias for renaming guest device to network device has been removed because it's no longer needed. In the UI, a breadcrumb_title is now passed to the nested_list method instead.

@miq-bot
Copy link
Member

miq-bot commented Apr 23, 2018

Checked commits skovic/manageiq@904c7dd~...dd7d0a9 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@skovic
Copy link
Author

skovic commented Apr 24, 2018

@miq-bot add_label providers/physical

@skovic
Copy link
Author

skovic commented Apr 24, 2018

@agrare This PR is ready for review. Thanks

@agrare
Copy link
Member

agrare commented Apr 24, 2018

I'm good with the scope, @himdel is removing that from locale/en.yml going to be a problem?

@himdel
Copy link
Contributor

himdel commented Apr 25, 2018

is removing that from locale/en.yml going to be a problem?

Pretty sure it will mean every GuestDevice will appear as "Guest Device" instead of "Network Device" in every generic part of the UI, and all the translations will change from whatever "Network Device" was translated to, to "Guest Device".

So, yeah, sounds like a problem.

Also, logically, if you have 2 different kinds of GuestDevice, should they really be instances of the same class?

Can we get this as 2 different classes instead? (Or do we just need a translation that doesn't say "network"?)

@agrare
Copy link
Member

agrare commented Apr 25, 2018

Hm well calling all GuestDevices "Network Devices" wasn't really correct in the first place since hosts have always had network and storage guest devices.

@himdel
Copy link
Contributor

himdel commented Apr 25, 2018

Well, if you're sure the classing is correct...

Ok, in that case, we need to keep a string in locale/en.yml but make it one generic enough to be used anywhere, for both kinds. (There is no generic way to distinguish between different kinds of GuestDevice in the UI if they share the same class.)

@agrare
Copy link
Member

agrare commented Apr 25, 2018

Well, if you're sure the classing is correct...

I'm not a fan of having both in the same model without at least STI but that is how it is today :/

@agrare agrare self-assigned this Apr 25, 2018
@skovic
Copy link
Author

skovic commented Apr 25, 2018

@agrare So are any changes required in the en.yml file? Or is it OK as is? I agree that it shouldn't be renamed to Network Device since guest devices can be network devices, storage device, etc. which is why I removed it here. Thanks

@agrare
Copy link
Member

agrare commented Apr 25, 2018

@skovic it sounds like @himdel said we still need a string for Guest Devices in locale/en.yml which will work for both ethernet and storage types. I/O device maybe? IDK i'm terrible at naming things :)

@skovic
Copy link
Author

skovic commented Apr 25, 2018

@agrare @himdel Can we just call it Guest Device like how GuestApplication is Guest Application and IsoDatastore is ISO Datastore?

@agrare
Copy link
Member

agrare commented Apr 25, 2018

@skovic that's okay with me

@himdel
Copy link
Contributor

himdel commented Apr 25, 2018

¯\_(ツ)_/¯ Fine by me :)

(If Guest Device is indeed the desired name, you don't actually need that locale/en.yml entry after all.)

((unless I'm wrong, @mzazrivec ?))

@skovic
Copy link
Author

skovic commented Apr 25, 2018

@himdel @agrare OK, I guess we can just leave the change for en.yml as is in this PR since the entry isn't needed.

@agrare
Copy link
Member

agrare commented Apr 25, 2018

In that case LGTM

@agrare agrare merged commit 51f8618 into ManageIQ:master Apr 25, 2018
@agrare agrare added this to the Sprint 85 Ending May 7, 2018 milestone Apr 25, 2018
@mzazrivec
Copy link
Contributor

We still need the entry in locale/en.yml, regardless of what it actually says (Network Device or Guest Device). We need it for translations to work correctly.

@skovic
Copy link
Author

skovic commented Apr 26, 2018

@mzazrivec @agrare @himdel OK, I've created a PR that adds an entry for guest device to en.yml. It's PR #17350

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