-
Notifications
You must be signed in to change notification settings - Fork 9
Add service and scope to query params in addition to headers
#282
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
Add service and scope to query params in addition to headers
#282
Conversation
|
@microsoft-github-policy-service agree |
|
I'm unsure on whether I should add to the changelog while others have open PRs with changes to it. |
service and scope to query params in addition to headers
bwateratmsft
left a comment
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 think your approach is good (honestly--probably better than what I did originally), but it goes against the pattern for setting the query params that we use in a few other places in this package. I should probably refactor it later towards your pattern, but for now I'd prefer to keep it consistent with the others. Can you change it to match the pattern below?
Here you can see it being set:
Lines 120 to 122 in 0f54cfc
| let requestUrl: vscode.Uri | undefined = DockerHubRequestUrl | |
| .with({ path: `v2/repositories/${registry.label}` }) | |
| .with({ query: pageSizeQuery }); |
And here's how that pageSizeQuery is defined:
Line 32 in 0f54cfc
| const pageSizeQuery = new URLSearchParams({ page_size: '100' }).toString(); |
In the toString for your resultant vscode.Uri, don't forget to pass true, otherwise it gets overencoded.
|
This looks good but im unsure where you want it implemented. Do you want it implemented for httpRequest or in the function calling it |
|
I was thinking in the caller of |
bwateratmsft
left a comment
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.
Looks good. I'll try to test it out this week.
|
Merged, thanks for the contribution @meixnerlu! I'm going to bump the package version and changelog and get it ready to release. Then I'll integrate it into |
|
Thanks its been fun :) |
Fixes #279
I added the option for query parameters to the RequestLike interface and the httpRequest function.
I added the query with service and scope to be send on the token request.
I ran the lint and test commands.