Seleniumgrid exposedui templated#65
Open
robert-doyensec wants to merge 3 commits into
Open
Conversation
giacomo-doyensec
left a comment
There was a problem hiding this comment.
LGTM - in general I would keep out of the code all comments related to migration since they are relative to components being removed
| version: "2.0" | ||
| } | ||
|
|
||
| # finding ID changed from Java version to be more consistent with the rest of them. |
There was a problem hiding this comment.
Suggested change
| # finding ID changed from Java version to be more consistent with the rest of them. |
I would write this in the upstream PR and keep the code clean
Comment on lines
+43
to
+45
| # in the original java detector, it loops until the API is ready. | ||
| # We shouldn't do that in our plugins, and instead set up the testbed properly to account for it. | ||
| # Changed it to a fingerprint step and added multiple capitalizations just in case it varies. |
There was a problem hiding this comment.
Suggested change
| # in the original java detector, it loops until the API is ready. | |
| # We shouldn't do that in our plugins, and instead set up the testbed properly to account for it. | |
| # Changed it to a fingerprint step and added multiple capitalizations just in case it varies. |
Comment on lines
+144
to
+145
| # This is only out of band detection, had errors implementing the reflective version correctly | ||
| # that were taking a while to debug. |
There was a problem hiding this comment.
I would just mention that reflective was dropped in the migration on the PR
Suggested change
| # This is only out of band detection, had errors implementing the reflective version correctly | |
| # that were taking a while to debug. |
| name: "fingerprint_selenium_grid" | ||
| http_request: { | ||
| method: GET | ||
| uri: "/status" |
There was a problem hiding this comment.
Old detector was using different EP for fingerprint (prepending wd/hub). Just pointing that out, if it worked during your tests it's fine to me
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I removed the reflective detector logic because I had issues reproducing that part the way it was implemented previously. No changes to testbed.