Allow external uri to be configurable for components that support server functionality - #12491

[utamas]

SE nodes might be behind some sort of proxy exposed to hub on a different hostname(/ip) and/or port than component would by default report themselves (e.g.: hub and nodes are in different k8s clusters
and services are exposed via node ports).

Fixes #12491

Relevant update to docs is at SeleniumHQ/seleniumhq.github.io#1448

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.

[diemol]

I left a comment on the issue. Have you checked it?

[utamas]

@diemol yes (although, I haven't tagged you)

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1182189) 57.65% compared to head (268e6f7) 57.65%.
Report is 7 commits behind head on trunk.

❗ Current head 268e6f7 differs from pull request most recent head d1ef0f1. Consider uploading reports for the commit d1ef0f1 to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@           Coverage Diff           @@
##            trunk   #12508   +/-   ##
=======================================
  Coverage   57.65%   57.65%           
=======================================
  Files          86       86           
  Lines        5281     5281           
  Branches      208      208           
=======================================
  Hits         3045     3045           
  Misses       2028     2028           
  Partials      208      208           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[joerg1985]

I think the URI should be an URL, so external-url might be a better parameter. Currently i don't have access to a dev system for the next days, so i am not able to make a real review, sorry.

[utamas]

@joerg1985 I'll make changes later today/tomorrow

[titusfortner]

@utamas thanks for your work on this code. I'm trying to figure out what all we want to get into 4.14, does this have much more work to do?

[utamas]

Hi @titusfortner I think a simple renaming is the only thing that needs to be done. I'm starting that now.

[utamas]

@joerg1985 as I'm making my changes I can now see why I picked external-uri. The property I needed
to make a slight adjustment is called: org.openqa.selenium.grid.server.BaseServerOptions#getExternalUri.

Despite this I think URL is a more accurate representation, hence made the changes.

[utamas]

@titusfortner and @joerg1985 please let me know if anything else needs to get done.

[titusfortner]

@utamas yeah, looks like we have some failures in our Remote Build Java tests:

Executing tests from //java/test/org/openqa/selenium/grid/server:BaseServerOptionsTest
-----------------------------------------------------------------------------
Failures: 1
1) externalUriFailsForNonUriStrings() (org.openqa.selenium.grid.server.BaseServerOptionsTest)
org.opentest4j.AssertionFailedError: [External URI must be parseable as URI.] 
expected: "Supplied external URI is invalid: Illegal character in path at index 3: not a uri"
 but was: "Supplied external URI is invalid: Illegal character in path at index 3: not a URL"

[utamas]

@titusfortner my bad. Run the test after rebase but not after changing the name.

[utamas]

@titusfortner how can I make sure this makes it into next release?

[titusfortner]

@joerg1985 can you take a look at this one?

[joerg1985]

it look good to me, but i never approved a PR. so i am not totally sure what i have to check in this case.

[utamas]

Thank you guys!