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

Adding "Management Console" access to open the VM's page on IBM Cloud #396

Merged
merged 1 commit into from Jun 24, 2022

Conversation

miyamotoh
Copy link
Contributor

@miyamotoh miyamotoh commented Jun 16, 2022

Adding Access > Native Console action for a PowerVS VM. Very hacky (and thus WIP and likely change), but can open the VM's page on IBM Cloud.

Signed-off-by: Hiro Miyamoto miyamotoh@us.ibm.com

UI PR: ManageIQ/manageiq-ui-classic#8319

@agrare
Copy link
Member

agrare commented Jun 17, 2022

@miyamotoh are you trying to open the VM's remote console or the details/dashboard page?

@Fryguy
Copy link
Member

Fryguy commented Jun 17, 2022

Yeah, I don't like the word "console" for something that is not like a terminal console or a remote desktop to a VM. Think I had a similar problem in the original PR, but I can't think of a better term, and I hadn't thought of the potential terminology clash on the VM page.

EDIT: Meant to put this on the other PR, so moving it there.

@miyamotoh
Copy link
Contributor Author

miyamotoh commented Jun 17, 2022

@agrare What I'm trying to open is a page on IBM Cloud (outside ManageIQ), showing the meta/details of the VM. See the screenshot in the other UI PR: ManageIQ/manageiq-ui-classic#8319

@agrare
Copy link
Member

agrare commented Jun 17, 2022

@miyamotoh yeah so the methods you're implementing are intended for "Remote Console" no the dashboard/details page.

For the VM dashboard you just need:

supports :native_console

def console_url
  "https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?paneId=manage&crn=#{crn}"
end

I didn't realize until very recently that Ovirt Remote Console had methods also called native_console when we were doing the PowerVS "open the native provider dashboard" work.

@miyamotoh
Copy link
Contributor Author

@agrare if that's all I need on the provider side, that's great, but what about the UI side? I wouldn't get the "Native Console" menu item nor any standalone button on the page. How does it get picked up by UI?

@agrare
Copy link
Member

agrare commented Jun 17, 2022

@miyamotoh correct but this is the provider PR 😉 I'll comment in the UI PR about what I think needs to happen there

@Fryguy
Copy link
Member

Fryguy commented Jun 17, 2022

Updated the OP to cross-link the PRs

@@ -104,6 +107,15 @@ def console_supported?(type)
return true if type.upcase == 'VNC'
end

def console_url
crn = CGI.escape(ext_management_system.pcloud_crn.values.join(":"))
Copy link
Member

Choose a reason for hiding this comment

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

As annoying as this is, CGI.escape should not be used because it doesn't handle things correctly...prefer ERB::Util.url_encode.

Aside, I'm tempted to put this in more_core_extensions so we don't have to think about this as much in the future.

Copy link
Member

Choose a reason for hiding this comment

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

I recently used CGI.escape here. Sounds like it should be avoided in general?

@@ -104,6 +107,15 @@ def console_supported?(type)
return true if type.upcase == 'VNC'
end

def console_url
crn = CGI.escape(ext_management_system.pcloud_crn.values.join(":"))
"https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?paneId=manage&crn=#{crn}"
Copy link
Member

@Fryguy Fryguy Jun 23, 2022

Choose a reason for hiding this comment

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

Actually, I just realized you are using this for building the query part of the URL...in that case, I think you should use URI.encode_www_form:

Suggested change
"https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?paneId=manage&crn=#{crn}"
params = URI.encode_www_form(:paneId => "manageiq", :crn => ext_management_system.pcloud_crn.values.join(":"))
"https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?#{params}"

Copy link
Member

Choose a reason for hiding this comment

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

Then as a optional followup, URL building should probably be done with the URI build method...something like:

Suggested change
"https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?paneId=manage&crn=#{crn}"
params = URI.encode_www_form(:paneId => "manageiq", :crn => ext_management_system.pcloud_crn.values.join(":"))
"https://cloud.ibm.com/services/power-iaas/#{crn}/server/#{uid_ems}/?#{params}"
URI::HTTPS.build(:host => "cloud.ibm.com", :path => "/services/power-iaas/#{crn}/server/#{uid_ems}", :query => params)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Feedback incorporated, force-push'd. PTAL.

@miyamotoh miyamotoh force-pushed the vm-native-console branch 3 times, most recently from ad644e0 to c7a5871 Compare June 23, 2022 18:24
@Fryguy
Copy link
Member

Fryguy commented Jun 23, 2022

Looks nice - just need to make sure the caller of this can handle a URI object or String being returned (they can just do .to_s)

@miyamotoh miyamotoh changed the title [WIP] Adding "Native Console" access to open the VM's page on IBM Cloud Adding "Management Console" access to open the VM's page on IBM Cloud Jun 23, 2022
@miq-bot miq-bot removed the wip label Jun 23, 2022
Signed-off-by: Hiro Miyamoto <miyamotoh@us.ibm.com>
@miq-bot
Copy link
Member

miq-bot commented Jun 23, 2022

Checked commit miyamotoh@165ee9f with ruby 2.6.9, rubocop 1.19.1, haml-lint 0.35.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@agrare agrare merged commit 3f38b09 into ManageIQ:master Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants