Skip to content

fix: update chart downloading logic and improve digest handling in im…#3080

Merged
redscholar merged 1 commit intokubesphere:mainfrom
redscholar:download_charts
Apr 22, 2026
Merged

fix: update chart downloading logic and improve digest handling in im…#3080
redscholar merged 1 commit intokubesphere:mainfrom
redscholar:download_charts

Conversation

@redscholar
Copy link
Copy Markdown
Contributor

…age repository

What type of PR is this?

/kind bug

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for reviewers:

Does this PR introduced a user-facing change?

none

Additional documentation, usage docs, etc.:


…age repository

Signed-off-by: redscholar <blacktiledhouse@gmail.com>
@kubesphere-prow kubesphere-prow Bot added release-note-none kind/bug Categorizes issue or PR as related to a bug. labels Apr 21, 2026
@kubesphere-prow
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: redscholar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubesphere-prow kubesphere-prow Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 21, 2026
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
53.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the Helm chart download process to support both OCI and standard HTTP repositories using helm pull and updates the image repository module to resolve manifest digests from tags using a local layout file. Feedback focuses on improving the robustness of these changes, specifically by handling missing tags in the layout file to avoid 500 errors, correcting log paths, quoting shell variables in Ansible tasks, and restoring idempotency checks to prevent redundant chart downloads.

Comment on lines +449 to +450
actualDigest = tagDigest[requestDigest]
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

If the requestDigest (the tag) is not found in the layout file, actualDigest will be an empty string. This causes actualFilename to point to the directory itself. os.Stat will succeed on the directory, but os.ReadFile will subsequently fail with an error (e.g., "is a directory"), leading to a 500 Internal Server Error. It is more appropriate to return responseNotFound (404) when the tag cannot be resolved.

			actualDigest = tagDigest[requestDigest]
			if actualDigest == "" {
				return responseNotFound
			}
		}

file, err := os.ReadFile(filename)
file, err := os.ReadFile(actualFilename)
if err != nil {
klog.V(4).ErrorS(err, "failed to read file", "filename", filename)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The error log should reference actualFilename instead of filename, as actualFilename is the path that the code attempted to read and failed.

Suggested change
klog.V(4).ErrorS(err, "failed to read file", "filename", filename)
klog.V(4).ErrorS(err, "failed to read file", "filename", actualFilename)

Comment on lines +30 to +31
url={{ .item.url }}
version={{ .item.version }}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Shell variables should be double-quoted to ensure the script handles URLs or versions containing spaces or special characters correctly.

    url="{{ .item.url }}"
    version="{{ .item.version }}"

copy:
src: "{{ .work_dir }}/charts/{{ base .item }}"
dest: "{{ .artifact_dir }}/charts/{{ base .item }}" No newline at end of file
command: |
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The transition from a copy task with a when condition to a command task has removed the idempotency check. helm pull will now be executed on every run, regardless of whether the chart already exists in the artifact directory. Consider adding a shell-level check to skip the download if the target file is already present to improve efficiency.

@redscholar redscholar merged commit 02b2aa1 into kubesphere:main Apr 22, 2026
5 of 7 checks passed
@redscholar redscholar deleted the download_charts branch April 22, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. release-note-none size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant