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

RHV Native viewer #19675

Merged
merged 1 commit into from
Jan 13, 2020
Merged

RHV Native viewer #19675

merged 1 commit into from
Jan 13, 2020

Conversation

gekorob
Copy link
Member

@gekorob gekorob commented Dec 24, 2019 •
edited
Loading

Add native viewer console to allow users download the
connection file from RHV instances.

Releted to BZ:
https://bugzilla.redhat.com/show_bug.cgi?id=1451594

Depends on:
ManageIQ/manageiq-providers-ovirt#452

@gekorob
Copy link
Member Author

gekorob commented Dec 24, 2019

@miq-bot add_label wip

@miq-bot miq-bot changed the title RHV Native viewer [WIP] RHV Native viewer Dec 24, 2019
@miq-bot miq-bot added the wip label Dec 24, 2019
@gekorob gekorob mentioned this pull request Dec 24, 2019
Merged
@gekorob
Copy link
Member Author

gekorob commented Jan 7, 2020

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] RHV Native viewer RHV Native viewer Jan 7, 2020
@miq-bot miq-bot removed the wip label Jan 7, 2020
@gekorob gekorob mentioned this pull request Jan 7, 2020
Merged
@skateman
Copy link
Member

skateman commented Jan 8, 2020

cc @mzazrivec for the i18n as mentioned here ManageIQ/manageiq-ui-classic#6574 (comment)

But I don't really understand the question, can you please elaborate @gekorob?

@gekorob
Copy link
Member Author

gekorob commented Jan 8, 2020

I was asking if I have to add translations for tooltips, button label or messages related to native console

@skateman
Copy link
Member

skateman commented Jan 9, 2020

You don't have to do more than you have with the _ and N_, it's all sorted out by automated tasks.

@skateman
Copy link
Member

skateman commented Jan 9, 2020

Tested this and it downloads the vv file. I don't really know what to do with it, but I guess it's irrelevant from the perspective of MiQ.

skateman
skateman approved these changes Jan 9, 2020
View reviewed changes
Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

chessbyte
chessbyte reviewed Jan 9, 2020
View reviewed changes
app/models/vm/operations.rb Outdated
@@ -18,6 +18,10 @@ module Vm::Operations
unsupported_reason_add(:vmrc_console, _("VMRC Console not supported")) unless console_supported?('VMRC')
end

supports :native_console do
unsupported_reason_add(:native_console, _("NATIVE Console not supported")) unless console_supported?('NATIVE')
Copy link
Member

@chessbyte chessbyte Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Should this say "VM NATIVE Console not supported" to be consistent with line 40 in this file?

Copy link
Member Author

@gekorob gekorob Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I'll change it like the one in line 40

@gekorob gekorob force-pushed the rfe/rhv_native_viewer branch 2 times, most recently from 6e51b06 to f4659eb Compare January 10, 2020 14:06
@martinpovolny
Copy link
Member

martinpovolny commented Jan 13, 2020 •
edited
Loading

Please, fix the last style issue so that we can merge this.

app/models/vm/operations.rb
  Line 39, Col 5 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

Thx!

@gekorob
RHV Native viewer
2586a45 
Add native viewer console to allow users download the
connection file from RHV instances.
@miq-bot
Copy link
Member

miq-bot commented Jan 13, 2020

Checked commit gekorob@2586a45 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 0 offenses detected
Everything looks fine. 🍰

@gekorob
Copy link
Member Author

gekorob commented Jan 13, 2020

@martinpovolny sure, sorry.
The exception raised by the validation method is a MiqException::RemoteConsoleNotSupportedError that is, at the end, a subclass of Ruby RuntimeError, but in the code that already exists the rescue with no parameters is referring to the StandardError, so for now I stayed with it.
Let me know if for you is ok.

@martinpovolny martinpovolny merged commit 66a0105 into ManageIQ:master Jan 13, 2020
@martinpovolny martinpovolny self-assigned this Jan 13, 2020
@martinpovolny martinpovolny added this to the Sprint 128 Ending Jan 20, 2020 milestone Jan 13, 2020
@skateman
Copy link
Member

Nice work 👍 but I think we should also add this to the SUI, should I create an issue for that?

@gekorob
Copy link
Member Author

gekorob commented Jan 14, 2020

@skateman I didn't add native_console into yaml section related to SUI 'cause I don't know too much about SUI and if the feature is already implemented.
But if I can be of any help just tell me

@skateman
Copy link
Member

Well, it's not, I'll create an issue.

@skateman skateman mentioned this pull request Jan 16, 2020
Closed
@mzazrivec mzazrivec mentioned this pull request Oct 1, 2020
Merged
@simaishi
Copy link
Contributor

simaishi commented Oct 6, 2020

Backported to ivanchuk via #20640

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chessbyte chessbyte chessbyte left review comments

@skateman skateman skateman approved these changes

Assignees

@martinpovolny martinpovolny

Labels
changelog/yes enhancement ivanchuk/backported providers/console
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gekorob @skateman @martinpovolny @miq-bot @simaishi @chessbyte