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

Remove flags for event-bus in start-selenium-grid-session-queue.sh after removal in 4.5.0 #1692

Merged

Conversation

efranken
Copy link
Contributor

@efranken efranken commented Oct 4, 2022

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

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

Description

Remove --publish-events and --subscribe-events flags from being passed in start-selenium-grid-sessions-queue.sh

Motivation and Context

As seen in 1687, if a user supplies an environment variables SE_EVENT_BUS_HOST, SE_EVENT_BUS_PUBLISH_PORT, and SE_EVENT_BUS_SUBSCRIBE_PORT, they are passed to selenium-server.jar as launch flags --publish-events and --subscribe-events. These flags were removed in 4.5.0, specifically this commit.

Currently, if a user with the aforementioned environment variables set runs the bash file, it will fail to launch with:
Was passed main parameter '--publish-events' but no main parameter was defined in your arg class. Removing the flags allows the session queue to launch.

I was unable to find any documentation that still referenced the --publish-events or --subscribe-events flags.

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.

@CLAassistant
Copy link

CLAassistant commented Oct 4, 2022

CLA assistant check
All committers have signed the CLA.

Event bus role was removed from session queue in this commit - SeleniumHQ/selenium@4b19fa4

Launching with the flags still in place causes an error and does not allow queue to start
@efranken efranken force-pushed the remove-eventbus-from-sessionqueue branch from 9986756 to ea959c7 Compare October 4, 2022 14:45
@efranken efranken marked this pull request as ready for review October 4, 2022 15:58
@jamesmortensen jamesmortensen merged commit ffec57e into SeleniumHQ:trunk Oct 4, 2022
@jamesmortensen
Copy link
Member

fixes #1687 Thank you @efranken I'm going to merge this now since it allows folks to use the session-queue again. If you're interested in opening another pull request, all of the docker-compose yaml files that use the session-queue need the SE_NODE environment variables removed, since they aren't doing anything anymore, at least not for the session-queue.

@efranken
Copy link
Contributor Author

efranken commented Oct 4, 2022

fixes #1687 Thank you @efranken I'm going to merge this now since it allows folks to use the session-queue again. If you're interested in opening another pull request, all of the docker-compose yaml files that use the session-queue need the SE_NODE environment variables removed, since they aren't doing anything anymore, at least not for the session-queue.

Sure thing, Im on it, need to research the codebase to make sure I do it properly as I don't use those specific files but I'll put up a new PR when I'm done

MantasGurskis pushed a commit to hostinger/docker-selenium that referenced this pull request Apr 26, 2023
…ter removal in 4.5.0 (SeleniumHQ#1692)

Removed event bus, not used in 4.5.0

Event bus role was removed from session queue in this commit - SeleniumHQ/selenium@4b19fa4

Launching with the flags still in place causes an error and does not allow queue to start
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

3 participants