Skip to content
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

Port HTTP agent ServiceClientOption to 1.x #404

Merged
merged 6 commits into from
Oct 8, 2020
Merged

Port HTTP agent ServiceClientOption to 1.x #404

merged 6 commits into from
Oct 8, 2020

Conversation

joshgummersall
Copy link
Contributor

Ports functionality to 1.x from:
https://github.com/Azure/ms-rest-js/pull/403/files

@joshgummersall
Copy link
Contributor Author

Hey @daviwil and @xirzec - I tested this over the weekend with the Bot Builder SDK and it looks good. Any chance we could get this change landed relatively soon? I'm happy to address any issues in this PR or the PR opened against master.

Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

lib/util/constants.ts Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@ramya-rao-a
Copy link
Contributor

@joshgummersall Is this related to the @azure/arm-botservice package? If so, we will soon be publishing an update for it which will use v2 of @azure/ms-rest-js package. In that case, would you still need this fix for v1 of @azure/ms-rest-js?

@joshgummersall
Copy link
Contributor Author

joshgummersall commented Oct 8, 2020

It’s for https://github.com/microsoft/botbuilder-js.

@ramya-rao-a
Copy link
Contributor

Thanks @joshgummersall

Any reason we cannot have the botbuilder-js project use v2 of @azure/ms-rest-js?

@joshgummersall
Copy link
Contributor Author

joshgummersall commented Oct 8, 2020

There is more context in #402. It boils down to backwards compatibility concerns. I added this same functionality to 2.x here (#403) as well as it is desirable functionality in both versions. Unfortunately, we are simply not able to upgrade to 2.x.

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good, can you also update Changelog.md to mention the new feature?

@joshgummersall
Copy link
Contributor Author

@daviwil all set.

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @joshgummersall!

@daviwil daviwil merged commit 5db0fcd into Azure:1.x Oct 8, 2020
@joshgummersall
Copy link
Contributor Author

Thanks for the reviews! Much appreciated.

@joshgummersall
Copy link
Contributor Author

@daviwil any luck with getting this published?

@daviwil
Copy link
Contributor

daviwil commented Oct 12, 2020

Sorry for the delay, I've got to fix a problem with our release automation to get this out, will get back to that as soon as I take care of some other pressing issues.

@joshgummersall
Copy link
Contributor Author

Thanks for the update. Looking forward to the release!

@daviwil
Copy link
Contributor

daviwil commented Oct 13, 2020

2.1.0 and 1.9.0 are both out now. Thanks again for the PRs!

@joshgummersall
Copy link
Contributor Author

Awesome, glad to contribute. Cheers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants