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(helm): allows users to define an existing secret for tokens #2587

Merged
merged 2 commits into from
Jul 31, 2022

Conversation

cebidhem
Copy link
Contributor

@cebidhem cebidhem commented Jul 25, 2022

Signed-off-by: cebidhem cebidhem@pm.me

Description

This will allow the users of the Helm chart to define an existing secret containing the env variables GITHUB_TOKEN TRIVY_TOKEN TRIVY_USERNAME TRIVY_PASSWORD as it is quite common for us to use external tools for creating secrets (could be external-secret or something else).
This would allow us to avoid putting tokens in a git repo.

I haven't seen a helm doc generator - but it's my very first PR here so I may have missed it - but I added it manually in the README.md of the chart.

I bumped the appVersion as it was quite old. Let me know if you'd rather do it in a separate PR.
I also bumped the Chart version, because I haven't seen a job under ci/ doing it. Let me know if it was wrong.

I tested locally with helm template, again let me know if I should add anything else.

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: cebidhem <cebidhem@pm.me>
@cebidhem cebidhem requested a review from krol3 as a code owner July 25, 2022 20:25
@CLAassistant
Copy link

CLAassistant commented Jul 25, 2022

CLA assistant check
All committers have signed the CLA.

@knqyf263
Copy link
Collaborator

Thanks! @krol3 Could you review this PR?

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 @cebidhem ! Thank you for your contribution, I suggestion about the version. It's a small change

@cebidhem
Copy link
Contributor Author

Hi @cebidhem ! Thank you for your contribution, I suggestion about the version. It's a small change

Hey @krol3, I can't see the suggestion in this PR thread. Would you want me to bump only the fix version ? If so, I'd argue that it adds a new feature to the chart, and that would be a good reason to bump the minor version imho.
But let me know, I'll do what you think is best with regards to the practices of the project.

@cebidhem cebidhem requested a review from krol3 July 28, 2022 19:44
@cebidhem
Copy link
Contributor Author

HI @krol3 ,

Do you think you'll have a chance to do the review today ? see my previous comment, if you left any suggestion/comment on the code, I can't see it in the code UI nor the comment thread.

It's kind of a blocker for us to start the implementation with the chart.

Thank you for your time

helm/trivy/Chart.yaml Outdated Show resolved Hide resolved
helm/trivy/Chart.yaml Outdated Show resolved Hide resolved
helm/trivy/README.md Outdated Show resolved Hide resolved
Signed-off-by: cebidhem <cebidhem@pm.me>
@cebidhem
Copy link
Contributor Author

Thanks for the review @krol3! I will let you resolve the conversations as you're the codeowner when you'll be happy with it.

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 Thank you @cebidhem for the contribution

@knqyf263 knqyf263 merged commit d0ca610 into aquasecurity:main Jul 31, 2022
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(helm): allow an user defined existing secret
4 participants