-
-
Notifications
You must be signed in to change notification settings - Fork 148
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
Replace deprecated HttpModule from @nestjs/common with @nestjs/axios #651
Replace deprecated HttpModule from @nestjs/common with @nestjs/axios #651
Conversation
Any plans to merge this PR? |
@wing328 @kay-schecker |
@feodorar when can this PR be merged? |
@rishabh1212 That is out of my hands, I'm afraid. I just did a peer review... |
Sorry for the delay in reviewing this change. Please take a look at the build errors. Not sure if it's related to 49b383b. Once resolved, I'll try to handle it with higher priority. |
I think that's probably the reason. Please update the github workflow accordingly as part of this PR. Thank you. |
@wing328 The problem with the failing pipeline was me not having executed yarn install properly which made "yarn --frozen-lock" fail because of differences in the yarn.lock-file. Now that I did that properly and fixed the yarn.lock-file it should be successful. Although the node version should not have been a problem, I looked into it. I upgraded all github actions and it looks like you use a matrix build strategy but did not define the values to be used for the node-version. I added the node version 16.x which should use version 16 with the latest minor and patch version. So in the future it can be assured to run with multiple versions. Seems as if there is an approval needed for the pipeline to run, so let's see if it is successful now. |
@wing328 |
No idea why the last step stuck at build state. I've gone ahead to merge it. Let's see how that goes. Thanks again for the PR. |
As the new axios HttpModules respects the environment variables HTTP_PROXY, HTTPS_PROXY and NO_PROXY, there is no need to configure it explicitly anymore.
This fixes issue #390