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] Call
proxy-tool
through dev-tool #18322[Unified Recorder] Call
proxy-tool
through dev-tool #18322Changes from 17 commits
15ed59e
f5a9992
946bf0b
29ad0f3
4bbc37e
fc25043
4f0f2e6
09a20ea
e69fa22
be70341
0938dc7
950f46b
fcd64ff
074bfe4
5f407f9
ce99f54
cb3342d
4285651
4adb560
acd56a9
ddd3b7b
a7147c2
da195a1
378a342
803a205
91f8a53
1c46de2
4469c36
d204966
b986e5f
0be5730
25968e7
bc48123
f2e1fbf
b0cda6a
77e8229
660ae89
a6d1dea
63cdb75
94e4e0d
6ce7ec8
f6a62c6
41fcb8d
dd0161c
7af4f16
210ed4d
bed8368
79ddd0f
ad06342
2f1ab3e
3817355
c3e44cb
788a308
e4f1ed0
881b3fc
baeea7d
fce7d97
c40196d
80df60b
58fc8d8
b074e27
348e1c1
8dddf51
6713590
cb6c383
35a39a3
37d151d
e5d4ff0
a192f99
5258da3
a76fcc2
83f9d37
f124e79
2283626
b790604
9743b8f
213a315
99300d4
227e21c
7696173
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.
I'm in favor of using a simpler HTTP client in dev-tool such as axios.
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 the only HTTP request that is made in the whole of dev-tool.
I don't want to take a dependency on another package if this serves my purpose.
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.
Very well, When/if we have more HTTP usage I'd like to replace it with axios, node-fetch, etc. (something a little bit more "batteries-included").
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.
core-rest-pipelines should help 🤔 at least in the future
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.
That'll add circular dependencies @sadasant.
core packages depend on dev-tool
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 don't think we want to use core for incidental stuff in dev-tool. It's designed for the client libraries and unless we really have some kind of request pipeline in dev-tool where performance and consistent configurability with the clients is important, I think it'll be nicer to just use a simple API.
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.
Not if you pin a version 🤔 since dev-tools is only a devDependency. But I agree with Will 👍
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.
Can these ports be parametrized?
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.
Logged an issue for this at #15829, will tackle in a future PR.
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.
nit:
/etc
is usually for system configurations. maybe use another directory?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.
@scbedd
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.
Azure/azure-sdk-tools#2279
@HarshaNalluru the PR is submitted to sync the proxy tool tag update, I will need to change this script as well. That will keep everything in sync.
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.
Can these ports be parametrized?
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.
Logged an issue for this at #15829, will tackle in a future PR.
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.
Definitely like the idea of using sdk-type for more things. 👍