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.
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
[Unified Recorder] TestProxyClient takes test context to generate recordings at desired location #17388
[Unified Recorder] TestProxyClient takes test context to generate recordings at desired location #17388
Changes from 32 commits
e13ec0d
40c4a5a
30058f7
5a2509e
70e1bbb
6eb87d1
ccbd14c
47bb18a
05c00b0
1968f07
e950683
6cb397c
f927df5
25fa7a6
390cb3f
79c90da
d3bd3e0
1475dc2
b49b78e
5030832
eeb79cc
b71afda
3ce602d
75e96c2
c916456
fa2e266
e110ccf
a66b3fb
344ac2e
1c9f090
cc4420a
3703d8e
56fa188
51cbab0
91d46ad
28037ec
b863c43
98b8def
9b2db94
dace442
4f06aff
da830bc
613f803
9ab9e1b
c5855cd
5fe22e1
b743285
e942956
c08f8b1
b1d079c
6239815
e6c6b0a
912568b
a110c8f
53b24cb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is considered a privileged mode of operation for linux containers, so it should be clear that this disables network namespacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--net=host
doesn't work on windows.Switching to
host.docker.internal
which works on windows and mac(to access host's network) by default and by adding the host in linux with--add-host host.docker.internal:host-gateway
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is more about making it clear what
--net=host
does from a computer privileges standpoint. The sentence mentioned--net=host
before, so I just think that if you mention to use it it should also mention that this disables the network namespace entirely and lets the container freely talk to everything on your network.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I did apply your suggestion. :)
But upon more testing, realized that whatever I wrote here didn't work on Windows and had to look for different ways.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a builtin called
path.relative
that may replace this function.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, applied
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this can be replaced with something like
testAbsolutePath.split(path.sep)
and then looking through the resulting array by index.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this check seems redundant, if it's not "sdk" the
findRecordingsFolderPath()
would have failed already?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
path.join(...args).join(path.sep)
produce anything other than args?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double join?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about that, meant
path.join(...args).split(path.sep);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the args