Skip to content

Set Docker container locale env vars using capability#11629

Closed
jsa34 wants to merge 1 commit intoSeleniumHQ:trunkfrom
jsa34:set_locale
Closed

Set Docker container locale env vars using capability#11629
jsa34 wants to merge 1 commit intoSeleniumHQ:trunkfrom
jsa34:set_locale

Conversation

@jsa34
Copy link
Copy Markdown

@jsa34 jsa34 commented Feb 7, 2023

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.

Description

Set locale environment variable using capability "se:locale" (necessary for Linux environments due to Chrome's inconsistent setting of this across platforms - see https://developer.chrome.com/docs/extensions/reference/i18n/#how-to-set-browsers-locale)

Motivation and Context

Allows setting this variable consistently in the dynamic grid upon a new session request.

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.

@jsa34
Copy link
Copy Markdown
Author

jsa34 commented Feb 7, 2023

@diemol - my assumed fix didn't work for setting the language env variable in the binary wrapper of the selenium docker project - it set the value in the node container, but the dynamically created containers still have the default locale (en-US).

My gut is telling me this is a better, more reliable approach and is akin to the way the timezone is able to be set/overridden.

Any feedback would be much appreciated as this is very new to me.

@jsa34 jsa34 changed the title Set locale env variables using capability Set Docker container locale env vars using capability Feb 7, 2023
@diemol
Copy link
Copy Markdown
Member

diemol commented Feb 8, 2023

OK, so it worked when you used it in standalone mode but not in Dynamic. How did you start the Standalone mode and how did you start the test?

@jsa34
Copy link
Copy Markdown
Author

jsa34 commented Feb 8, 2023

So... I realised after re-testing why it wasn't working after the release that it was because I was setting environment variables in the ansible script I was using for deploying the node containers for LANGUAGE (and LANG and LC_ALL) (hence the env variables were as expected!), so made a rookie error QA-ing, despite being a QA-er! Apologies.

@diemol
Copy link
Copy Markdown
Member

diemol commented Feb 8, 2023

So... I realised after re-testing why it wasn't working after the release that it was because I was setting environment variables in the ansible script I was using for deploying the node containers for LANGUAGE (and LANG and LC_ALL) (hence the env variables were as expected!), so made a rookie error QA-ing, despite being a QA-er! Apologies.

Sorry, I am not following. Does it mean it is working now?

@jsa34
Copy link
Copy Markdown
Author

jsa34 commented Feb 8, 2023

No - in short, my previous fix didn't work.

Looking at how the dynamic grid creates the node containers, though, I thought this would be a more suitable place to change how the environment variables are set. (I think this is how the dynamic node containers are generated)

@diemol
Copy link
Copy Markdown
Member

diemol commented Mar 7, 2023

@jsa34 I made the change where you suggested it, thank you for digging into it and giving pointers where to fix it. Now the parent container will use its LANGUAGE env var to set the language in the child ones.

I thought about having it as a capability but this use case is not common and creates more maintenance.

alpatron pushed a commit to alpatron/selenium that referenced this pull request Mar 15, 2023
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.

2 participants