Navigation Menu

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

Jump to bottom

feat(helm): make cache TTL configurable #2798

Merged
merged 2 commits into from Sep 8, 2022
Merged

feat(helm): make cache TTL configurable #2798

merged 2 commits into from Sep 8, 2022

Conversation

elchenberg
Copy link
Contributor

@elchenberg elchenberg commented Aug 31, 2022 •
edited

Description

Enables users to configure the cache TTL with a new helm value.

Related issues

Checklist

  • I've read the guidelines for contributing to this repository.
  • I've followed the conventions in the PR title.
  • I've added tests that prove my fix is effective or that my feature works.
  • I've updated the documentation with the relevant information (if needed).
  • I've added usage information (if the PR introduces new options)
  • I've included a "before" and "after" example to the description (if the PR is a user interface change).

Signed-off-by: elchenberg elchenberg@users.noreply.github.com

@elchenberg
helm: make cache TTL configurable
cec0b63 
Signed-off-by: elchenberg <elchenberg@users.noreply.github.com>
@elchenberg elchenberg requested a review from krol3 as a code owner August 31, 2022 14:39
@CLAassistant
Copy link

CLAassistant commented Aug 31, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@elchenberg
Copy link
Contributor Author

Regarding the CLA: The CLA assistant asks me to sign a "SAP Individual Contributor License Agreement" (SAP as in the german software company). That does not seem right.

Regarding the PR title: I am not sure what type of PR this is. A feature? A bugfix? A chore? Or something else?

@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 1, 2022 •
edited

Regarding the PR title: I am not sure what type of PR this is. A feature? A bugfix? A chore? Or something else?

It is a feature.

@elchenberg Oh, thanks for letting us know. It looks like something wrong.

@itaysk Can you take a look?

Regarding the CLA: The CLA assistant asks me to sign a "SAP Individual Contributor License Agreement" (SAP as in the german software company). That does not seem right.

@elchenberg elchenberg changed the title helm: make cache TTL configurable feat(helm): make cache TTL configurable Sep 1, 2022
@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 1, 2022

@elchenberg Seems like a bug of cla-assistant. We'll look into it later, but I reverted it for now. Could you try that again?

@knqyf263
Copy link
Collaborator

knqyf263 commented Sep 4, 2022

@krol3 Can you please review it?

krol3
krol3 suggested changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@krol3 krol3 left a comment

Choose a reason for hiding this comment

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

Hi @elchenberg , please only add in the readme this new parameter.

helm/trivy/templates/configmap.yaml
@@ -9,6 +9,7 @@ data:
TRIVY_CACHE_DIR: "/home/scanner/.cache/trivy"
{{- if .Values.trivy.cache.redis.enabled }}
TRIVY_CACHE_BACKEND: {{ .Values.trivy.cache.redis.url | quote }}
TRIVY_CACHE_TTL: {{ .Values.trivy.cache.redis.ttl | quote }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the readme- parameters section

| trivy.cache.redis.ttl | Specify redis ttl. e.g 3600s, 24h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I forgot about that!

@elchenberg
add usage instructions for trivy.cache.redis.ttl to the README
00164e2 
Signed-off-by: elchenberg <elchenberg@users.noreply.github.com>
krol3
krol3 approved these changes Sep 7, 2022
View reviewed changes
Copy link
Contributor

@krol3 krol3 left a comment

Choose a reason for hiding this comment

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

LGTM

@knqyf263 knqyf263 merged commit 41a8496 into aquasecurity:main Sep 8, 2022
@zetaab
Copy link

zetaab commented Nov 17, 2022 •
edited

@knqyf263 @elchenberg @krol3 its impossible to use these new things because chart versions are not bumped when you do changes to charts. Now the only way to use newest trivy helm chart things is to clone it locally and run. If you could https://github.com/aquasecurity/trivy/blob/main/helm/trivy/Chart.yaml#L4 increase this version, everyone could use these charts quite easily.

@zetaab
Copy link

zetaab commented Nov 17, 2022

also your ci pipelines is failing see: https://github.com/aquasecurity/trivy/actions/runs/3359974186/jobs/5568645331

Screenshot 2022-11-17 at 18 00 13

it fails because the version is not increased.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krol3 krol3 krol3 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Helm chart: make cache TTL configurable
5 participants
@elchenberg @CLAassistant @knqyf263 @zetaab @krol3