Skip to content

ability to set no_vnc_port#9927

Closed
therealdjryan wants to merge 3 commits intoSeleniumHQ:trunkfrom
therealdjryan:Set_NO_VNC_PORT
Closed

ability to set no_vnc_port#9927
therealdjryan wants to merge 3 commits intoSeleniumHQ:trunkfrom
therealdjryan:Set_NO_VNC_PORT

Conversation

@therealdjryan
Copy link
Copy Markdown

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Ability to set NO_VNC_PORT

Description

The vnc port is hardcoded to 7900. Allowing it to be set will enable live view for users who are not using the default port

Motivation and Context

Live view does not work for me because I am unable to set separate external and internal ports

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

The change simply reads the system property of NO_VNC_PORT and uses the default 7900 if not set. ** @diemol, I could change this to use a command line option instead of an environment variable, but I am not all that familiar with the codebase and NO_VNC_PORT is a variable already used in docker-selenium. **

@titusfortner titusfortner added the B-grid Everything grid and server related label Dec 24, 2021
Copy link
Copy Markdown
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

@therealdjryan, my apologies, took too long for me to review this.

I am sorry, so instead of asking you for changes, I implemented the configuration flag so you can use it in the next release. Commit is linked in this PR.

Hoping to see more contributions and comments in Slack.

@diemol diemol closed this Mar 4, 2022
elgatov pushed a commit to elgatov/selenium that referenced this pull request Jun 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-grid Everything grid and server related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants