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

Drop the vnc_port start, end and range from the codebase #2954

Closed
wants to merge 1 commit into from

Conversation

skateman
Copy link
Member

@skateman skateman commented Dec 5, 2017

This seems like dead code to me, so ✂️ 🗑️ ✂️ 🔥 ✂️ 🚽

@martinpovolny and @agrare can you confirm?

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

@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

Not quite sure, but looks like this is used in

manageiq-providers-vmware/app/models/manageiq/providers/vmware/infra_manager/host.rb

  def reserve_next_available_vnc_port
    port_start = ext_management_system.try(:host_default_vnc_port_start).try(:to_i) || 5900
    port_end   = ext_management_system.try(:host_default_vnc_port_end).try(:to_i) || 5999

    ...

called from remote_console_vnc_acquire_ticket.

The reason I'm not as sure is that that method is called by remote_console_html5_acquire_ticket ... which is never called :) (or at least not by the full name)

@miq-bot
Copy link
Member

miq-bot commented Dec 5, 2017

Checked commit skateman@c2cb0b1 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. ⭐

@skateman
Copy link
Member Author

skateman commented Dec 5, 2017

@himdel the remote_console_html5_acquire_ticket is actually being called, using send. My problem is that these ports are never being set and according to the BZ the fields are available only when editing a provider and not when adding it. In every case when I tested this the ports weren't set and the default ones have been used.

@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

Sounds like we should add the fields on adding as well :).

Maybe it may make sense to prefill them with the defualt values?

(Well, or if nobody is ever using it, drop the whole thing, even from the db, but.. do we know? :))

@skateman
Copy link
Member Author

skateman commented Dec 5, 2017

This is where @agrare or someone from the providers team could help us decide 😉

@agrare
Copy link
Member

agrare commented Dec 5, 2017

Hey @skateman what specifically do you need help deciding on?

@himdel
Copy link
Contributor

himdel commented Dec 5, 2017

@agrare The choice is:

  • drop the fields completely, from the form, the db, etc. and just use the defaults (5900 and 5999)
  • OR use the fields, add them to the new ems form, and maybe prefil them with the defaults so they're not empty.

Basically, the question is: do customers ever set it to non-default values?

@agrare
Copy link
Member

agrare commented Dec 5, 2017

Basically, the question is: do customers ever set it to non-default values?

I have to assume that yes, at some point, someone has set non-default values because security :)

@skateman
Copy link
Member Author

skateman commented Dec 5, 2017

Closing this PR then, will create a new one where we display the fields on a new vmware provider form too.

@skateman skateman closed this Dec 5, 2017
@skateman skateman deleted the drop-vmware-vnc-port branch December 5, 2017 21:39
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

4 participants