feat(1961): Replace request package with got #45
Conversation
| package-lock.json |
There was a problem hiding this comment.
remove package-lock
|
Created new library: https://github.com/screwdriver-cd/request |
| * @return {Promise} Resolves when no error encountered. | ||
| * Rejects when status code is non-200 | ||
| */ | ||
| function checkResponseError(response, caller) { |
There was a problem hiding this comment.
this has been moved to https://github.com/screwdriver-cd/request/blob/master/index.js
There was a problem hiding this comment.
Is retry still now handled like the PR description suggests ?
Also https://github.com/screwdriver-cd/request/blob/master/index.js#L50 shouldn't this be a number ?
|
|
||
| this.breaker = new Breaker(Request, this.config.fusebox); | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| this.breaker = new Breaker(this._gitlabCommand.bind(this), { |
There was a problem hiding this comment.
why continue to use breaker? Why not use the native retry capabilities of the library https://github.com/sindresorhus/got/blob/main/documentation/7-retry.md
Sine we are rewriting, we might as well not rely on a library like circuit-fuses which is still callback based.
There was a problem hiding this comment.
It was my understanding that circuit breaker vs retry are not exactly the same thing:
- https://dev.to/supriyasrivatsa/retry-vs-circuit-breaker-346o
- https://docs.microsoft.com/en-us/azure/architecture/patterns/circuit-breaker
- https://dantt.medium.com/circuit-breaker-and-retry-64830e71d0f6
It might be good to look for replacements or upgrades to the circuit-fuses library, but I think that is out of scope for this issue. Also, I'd rather not change two major packages at the same time.
index.js
Outdated
| qs: { | ||
| token, | ||
| route: `users`, | ||
| json: { |
There was a problem hiding this comment.
how come json is being passed for GET request ?
Also for we seem to be using https://github.com/sindresorhus/got/blob/main/documentation/2-options.md#allowgetbody , why ? This looks like we are changing how GET parameters are being passed passed from query string to request body, and is now at the mercy of http server to do the right thing.
There was a problem hiding this comment.
I thought we needed to pass qs as json but it should be searchParams
fixed: screwdriver-cd/request#4
Context
It would be nice to replace deprecated request package with a different one.
Objective
This PR attempts to replace request package with got package.
TODO:
References
Related to screwdriver-cd/screwdriver#1961
License
I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.