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

feat: Added retry option and max_retries #50

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

SaptarshiSarkar12
Copy link

Fixes

Fixes #49

Changes Proposed

  • Added retry_on_timeout boolean to toggle retry feature
  • Added max_retries to take the maximum number of retry as input

@SaptarshiSarkar12 SaptarshiSarkar12 force-pushed the 49-feat-add-retry-action-support branch from f7d3eb7 to 747cb1c Compare September 21, 2024 13:54
@SaptarshiSarkar12
Copy link
Author

@ashnamehrotra @sozercan Please review this PR

@ashnamehrotra
Copy link
Contributor

@SaptarshiSarkar12 thank you for the PR! I think we can simplify this to one argument. If there are a # of retries specified, we can retry otherwise we don't use retry logic? Would you also be able to modify the test to use these arguments to confirm it works as expected? https://github.com/project-copacetic/copa-action/blob/main/.github/workflows/build.yaml#L83

@SaptarshiSarkar12
Copy link
Author

SaptarshiSarkar12 commented Oct 5, 2024

I think we can simplify this to one argument. If there are a # of retries specified, we can retry otherwise we don't use retry logic?

Yes, correct 👍.

Would you also be able to modify the test to use these arguments to confirm it works as expected? https://github.com/project-copacetic/copa-action/blob/main/.github/workflows/build.yaml#L83

@ashnamehrotra Okay. I will 😀. I am out of station now. So, I would make the changes once I return back home (probably 14th of October). Is it fine?

Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
@SaptarshiSarkar12 SaptarshiSarkar12 force-pushed the 49-feat-add-retry-action-support branch from 10c2b75 to ba65fd0 Compare October 18, 2024 07:11
… would handle it from now onwards

Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
@SaptarshiSarkar12
Copy link
Author

@ashnamehrotra Hi Ashna 👋!
I have made the requested changes. Please review it.

Nowadays, I am encountering an issue with copa for the below situation.
Suppose all the vulnerabilities are already patched for oracle image. But Trivy reports false positives, so, the GH Action runs copa patch but it fails with the error Error: no patchable packages found.
A probable solution can be adding a boolean actions input called fail_if_no_vulns which will not exit with status code 1 if it fails for that reason. So, would I create a new issue for that?
Reference workflow run: https://github.com/SaptarshiSarkar12/Drifty/actions/runs/10968623586/job/30460218219#step:6:2111

@SaptarshiSarkar12
Copy link
Author

TOOMANYREQUESTS: retry-after: 569.492µs, allowed: 44000/minute

@ashnamehrotra From the failed build log, it seems that there have been too many requests error. Unfortunately, it is a bug reported in aquasecurity/trivy-action#389 and https://github.com/orgs/community/discussions/139074#discussioncomment-10808081 where they suggest using AWS image.
What to do from our side to fix this build? A retry might work 🤞.

@ashnamehrotra
Copy link
Contributor

@ashnamehrotra Hi Ashna 👋! I have made the requested changes. Please review it.

Nowadays, I am encountering an issue with copa for the below situation. Suppose all the vulnerabilities are already patched for oracle image. But Trivy reports false positives, so, the GH Action runs copa patch but it fails with the error Error: no patchable packages found. A probable solution can be adding a boolean actions input called fail_if_no_vulns which will not exit with status code 1 if it fails for that reason. So, would I create a new issue for that? Reference workflow run: https://github.com/SaptarshiSarkar12/Drifty/actions/runs/10968623586/job/30460218219#step:6:2111

Hi @SaptarshiSarkar12 this is something we will fix soon in project-copacetic/copacetic#802. This way we will be able to ignore the error of no upgradable packages with the --ignore-errors flag

entrypoint.sh Outdated Show resolved Hide resolved
entrypoint.sh Outdated Show resolved Hide resolved
Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
…cript

Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
@SaptarshiSarkar12
Copy link
Author

Hi @SaptarshiSarkar12 this is something we will fix soon in project-copacetic/copacetic#802. This way we will be able to ignore the error of no upgradable packages with the --ignore-errors flag

@ashnamehrotra Hi Ashna 👋!
That sounds great 👍. Would really appreciate such improvement.
But would that cause any issue with my current setup for patching oraclelinux - I'm currently patching the whole image with no Trivy reports passed to copa?

@ashnamehrotra
Copy link
Contributor

@SaptarshiSarkar12 it would only cause errors if there are no upgradable packages, so you would need to catch/ignore that error until we add support for that error via --ignore-errors.

To resolve the CI tests, you will also need to modify the bats test which runs the copa-action to take in the retry argument.

@SaptarshiSarkar12
Copy link
Author

@SaptarshiSarkar12 it would only cause errors if there are no upgradable packages, so you would need to catch/ignore that error until we add support for that error via --ignore-errors.

Okay, so when would that --ignore-errors flag be available?

To resolve the CI tests, you will also need to modify the bats test which runs the copa-action to take in the retry argument.

@ashnamehrotra I have added '5' as the last argument to the copa-action bats test step. Can you please check if that is correct or changes are required in any other section as well?

@ashnamehrotra
Copy link
Contributor

@SaptarshiSarkar12 it would only cause errors if there are no upgradable packages, so you would need to catch/ignore that error until we add support for that error via --ignore-errors.

Okay, so when would that --ignore-errors flag be available?

To resolve the CI tests, you will also need to modify the bats test which runs the copa-action to take in the retry argument.

@ashnamehrotra I have added '5' as the last argument to the copa-action bats test step. Can you please check if that is correct or changes are required in any other section as well?

We are currently prioritizing other issues https://github.com/project-copacetic/copacetic/milestone/8, so it will likely be available in the next milestone v0.11.0 release. It looks like build tests are still failing and the retry argument is not propagating. It would also be good to have a default value 0 set if no arg is provided.

@SaptarshiSarkar12
Copy link
Author

We are currently prioritizing other issues https://github.com/project-copacetic/copacetic/milestone/8, so it will likely be available in the next milestone v0.11.0 release.

@ashnamehrotra Okay. Thank you for prioritizing that issue for the next release 😄.

It looks like build tests are still failing and the retry argument is not propagating.

How do I fix that? Any clue?

It would also be good to have a default value 0 set if no arg is provided.

Okay, I'll add it and let's see if that fixes the build failure 🤞.

Signed-off-by: Saptarshi Sarkar <saptarshi.programmer@gmail.com>
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.

feat: Add support for retrying copa patch on failure/timeout
2 participants