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

Integrate retry mechanism #82

Merged
merged 12 commits into from
May 1, 2019
Merged

Integrate retry mechanism #82

merged 12 commits into from
May 1, 2019

Conversation

djomaa
Copy link
Contributor

@djomaa djomaa commented Apr 30, 2019

Fixes #75
Now if audit fails due to npm registry, it's repeated up to 5 times.

Probably, it would be cool to create parameters for retry delay and retry max attempts.

Copy link
Member

@quinnturner quinnturner left a comment

Choose a reason for hiding this comment

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

Thank you for your submission! I have outlined a few things to fix in my first pass.

As for the vulnerabilities blocking this, #83 should be merged. Once it is merged, we can merge master into your branch so that CircleCI registers the updated dependencies (to be addressed in #69).

lib/npm-auditer.js Outdated Show resolved Hide resolved
lib/yarn-auditer.js Outdated Show resolved Hide resolved
@quinnturner quinnturner requested a review from andy-patt May 1, 2019 05:23
@quinnturner
Copy link
Member

quinnturner commented May 1, 2019

I agree that the retry attempt should be configurable. Perhaps the following specification?

Arg: none
Alias: --retry-count
Description: The number of attempts audit-ci calls an unavailable registry before failing (default 5)

Placing this ^ after --registry in the various README options.

Config file spec:
"retry-count": // [Optional] defaults 5

Also, we should consider documenting this behaviour in the README.

If the registry's service is unavailable, audit-ci will re-run 5 times before failing unless otherwise specified with --retry-count. This behaviour has been noted in #75.

@djomaa
Copy link
Contributor Author

djomaa commented May 1, 2019

@quinnturner all is ready.

I also changed audit start flow for the availability of the mechanism in tests

@quinnturner
Copy link
Member

quinnturner commented May 1, 2019

Created djomaa#1 for some minor fixes along with a git merge master.

It would be a nice-to-have having a test that fails at first due to a 503 then passes. However, it's okay if that is added later.

Retry mechanism test fixes
@quinnturner quinnturner merged commit 76f1b8a into IBM:master May 1, 2019
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.

Audit randomly fails due to 503 (Yarn and NPM)
2 participants