apache / openwhisk-client-js

JavaScript client library for the Apache OpenWhisk platform
https://openwhisk.apache.org/
Apache License 2.0
82 stars 53 forks source link

Support client retries #227

Closed moritzraho closed 2 years ago

moritzraho commented 2 years ago

This PR adds support for retries in the client using the async-retry package. Like now, the default is no retries. Retries are performed on http errors and all 4xx and 5xx error codes. This can be re-discussed, e.g. retries on 4xx might not be wanted. Default values might be set differently too: https://github.com/apache/openwhisk-client-js/pull/227/files#diff-ca407d959d33ce92a7f7efee354a5ea5e6e61fcd797d32c04205e429a074ed54R160

This has been tested via unit tests and manually.

Api doc and README have been updated too.

moritzraho commented 2 years ago

anyone wants to look at this ? @rabbah maybe ?

moritzraho commented 2 years ago

Thanks for your reviews @selfxp and @rabbah ! Closed the comments, in for another quick review ?

rabbah commented 2 years ago

@moritzraho is there a missing commit with the recent changes?

moritzraho commented 2 years ago

@moritzraho is there a missing commit with the recent changes?

Yeah haha 😅

moritzraho commented 2 years ago

thanks for the review @rabbah

moritzraho commented 2 years ago

Can we merge this one ? cc @rabbah @selfxp

selfxp commented 2 years ago

It looks like there were some changes on how travis reports build statuses: https://github.com/travis-ci/travis-ci/issues/10204. @rabbah @dgrove-oss would you be able to make these git project changes as follows (i don't have access to project's settings):

- Go to the repository’s Branch Settings page (https://github.com/apache/openwhisk-client-js/settings/branches).
- In the "Branch Protection Settings" section, click "Edit" for a protected branch
- Scroll down to the second box, "Require status checks to pass before merging." Select either "Travis CI - Pull Request" or "Travis CI - Branch" or both. (Note: "continuous-integration/travis-ci" is the Status API event which is deprecated)
- Save your changes
dgrove-oss commented 2 years ago

We can't change things through the GitHub UI, but can configure via .asf.yaml. I believe it is already set correctly (https://github.com/apache/openwhisk-client-js/blob/master/.asf.yaml#L30-L38).

moritzraho commented 2 years ago

Maybe it's an issue in travis, anyone has access to verify ?

dgrove-oss commented 2 years ago

I noticed in travis.yml we are requesting a node10 environment, which is long out of support. I'm trying to drop that in #228 and see if that helps.

dgrove-oss commented 2 years ago

Travis is triggering now, but there's a failure. Wild guessing, I would speculate that the PR added some additional dependencies and we failed the size check.

> openwhisk@3.21.4 check-deps-size /home/travis/build/apache/openwhisk-client-js
2402> ./tools/check_size.sh 1100
2403
2404Checking node_modules size isn't more than threshold (kb): 1100
2405openwhisk-3.21.4.tgz
2406added 8 packages from 8 contributors and audited 747 packages in 8.63s
2407found 13 vulnerabilities (12 moderate, 1 high)
2408  run `npm audit fix` to fix them, or `npm audit` for details
2409failure! node_modules size (1104) is more than threshold
2410npm ERR! code ELIFECYCLE
2411npm ERR! errno 1
2412npm ERR! openwhisk@3.21.4 check-deps-size: `./tools/check_size.sh 1100`
2413npm ERR! Exit status 1
2414npm ERR! 
2415npm ERR! Failed at the openwhisk@3.21.4 check-deps-size script.
moritzraho commented 2 years ago

@dgrove-oss is travis triggered manually ?

dgrove-oss commented 2 years ago

travis is clean. @rabbah or @selfxp if you are happy with changes go ahead and merge. We did need to bump up the arbitrary limit of 1000 dependencies, but since async-retry was added that is probably not unexpected.

purplecabbage commented 2 years ago

Do we have an idea of when this would be released? @rabbah @dgrove-oss @selfxp

selfxp commented 2 years ago

I'm not on the PMC, @tysonnorris will help release this tomorrow morning