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(manager/helm-values): add image support in helm values manager #29828

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

Conversation

KyriosGN0
Copy link
Contributor

@KyriosGN0 KyriosGN0 commented Jun 24, 2024

Changes

This PR adds the image as viable option in helm-values manager
as some charts use image and not repository in the values.yaml

Context

Some of our internal charts use image instead of repository

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added/modified unit tests, or
  • No unit tests but ran on a real repository, or
  • Both unit tests + ran on a real repository

Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
@KyriosGN0 KyriosGN0 changed the title Feat/add image support in helm values manager feat(manager/helm-values): add image support in helm values manager Jun 24, 2024
Copy link
Member

@viceice viceice left a comment

Choose a reason for hiding this comment

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

  1. remove the closes keyword, this PR doesn't resolve that issue.

  2. please add a use case to the PR description.

lib/modules/manager/helm-values/extract.ts Show resolved Hide resolved
lib/modules/manager/helm-values/readme.md Outdated Show resolved Hide resolved
KyriosGN0 and others added 2 commits June 26, 2024 00:20
Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
Co-authored-by: Michael Kriese <michael.kriese@visualon.de>
@KyriosGN0
Copy link
Contributor Author

@viceice why does this PR doesn't close #7010 ?
what is it i am missing ?

Signed-off-by: Aviv Guiser <avivguiser@gmail.com>
@secustor
Copy link
Collaborator

@viceice why does this PR doesn't close #7010 ? what is it i am missing ?

You are implementing support for image and the issue is about name.

@KyriosGN0
Copy link
Contributor Author

@secustor fixed that!

@KyriosGN0 KyriosGN0 requested a review from viceice June 28, 2024 16:02
@@ -0,0 +1,68 @@
# Default values for test-chart.
Copy link
Member

Choose a reason for hiding this comment

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

Reduce to minimal size and inline to test

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean to inline? i should remove the file and pass the values in the test ?

Copy link
Member

Choose a reason for hiding this comment

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

yes

Comment on lines +14 to +17
const helmImageInsteadOfRepository = Fixtures.get(
'values_with_image_instead_of_repo.yaml',
);

Copy link
Member

Choose a reason for hiding this comment

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

inline to test

currentItem.image === undefined
) {
logger.debug('repository and image are both undefined');
return null;
Copy link
Member

Choose a reason for hiding this comment

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

needs a test

@rarkins
Copy link
Collaborator

rarkins commented Aug 21, 2024

@KyriosGN0 do you have time to continue this PR?

@KyriosGN0
Copy link
Contributor Author

@rarkins, yes i just don't understand what is it i should change ?

@rarkins
Copy link
Collaborator

rarkins commented Aug 21, 2024

@rarkins, yes i just don't understand what is it i should change ?

Removed the added fixture files. Instead reduce the content as much as possible, and then inline it in the spec.ts file using codeBlock syntax. Here's an example of one:

const parentProjectSrc = codeBlock`
(defproject org.example/parent-project "1.0.0-SNAPSHOT"
:plugins [[lein-parent "0.3.9"]
[lein-project-version "0.1.0"]
[lein-shell "0.5.0"]]
:parent-project {:coords [my-org/my-parent "4.3.0"]
:inherit [:profiles :managed-dependencies :local-repo]}
:profiles {:cljfmt {:plugins [[lein-cljfmt "0.9.2"]]}}
:dependencies [[org.clojure/core.async "1.6.681"]
[org.clojure/core.match "1.1.0"]
[org.clojure/data.csv "1.1.0"]
[org.clojure/tools.cli "1.1.230"]
[metosin/malli "0.15.0"]])`;

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.

None yet

4 participants