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

Proper support for caching helm chart from Artifactory. #5123

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

Lirt
Copy link
Contributor

@Lirt Lirt commented Feb 4, 2025

Currently helm index of ceph helm repository configures URLs to charts with relative path (see https://ceph.github.io/csi-charts/index.yaml).

Artifactory contains support for virtual repositories. Virtual repositories are able to cache remote repositories and provide offline access to those repositories.

In order for this to work correctly, charts must specify absolute base URL to artifacts.

This commit adds this base URL using argument for helm index command.

URL with previous approach:

    urls:
    - cephfs/ceph-csi-cephfs-3.13.0.tgz

URL with current approach (my personal "lirt" repo was used to test this, the MR specifies correct "ceph" path)

    urls:
    - https://lirt.github.io/csi-charts/cephfs/ceph-csi-cephfs-3.13.0.tgz

Is there anything that requires special attention

This change is backwards compatible.

You can try following commands that suggest the path is the same as previously and work with original repo and updated repo as well. Of course the MR specifies ceph. prefix.

helm pull https://lirt.github.io/csi-charts/cephfs/ceph-csi-cephfs-3.13.0.tgz
helm pull https://ceph.github.io/csi-charts/cephfs/ceph-csi-cephfs-3.13.0.tgz

Checklist:

  • Commit Message Formatting: Commit titles and messages follow
    guidelines in the developer
    guide
    .
  • Reviewed the developer guide on Submitting a Pull
    Request
  • Pending release
    notes

    updated with breaking and/or notable changes for the next major release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@mergify mergify bot added the component/deployment Helm chart, kubernetes templates and configuration Issues/PRs label Feb 4, 2025
@Lirt Lirt force-pushed the compatibility/helm-artifactory branch from 919b8c1 to f7374e2 Compare February 4, 2025 15:24
@nixpanic nixpanic added the ci/skip/multi-arch-build skip building on multiple architectures label Feb 4, 2025
@nixpanic
Copy link
Member

nixpanic commented Feb 4, 2025

Hi @Lirt , thanks for the PR!

This looks like a reasonable change to me, and you explained the details well. There are two problems that the commit checker bot complains about:

✖ header must not be longer than 72 characters, current length is 73 [header-max-length]
✖ body's lines must not be longer than 80 characters [body-max-line-length]

Can you amend your commit message and force-push the branch again?

@Lirt Lirt force-pushed the compatibility/helm-artifactory branch from f7374e2 to 0944d65 Compare February 5, 2025 08:46
Currently helm index configures URLs to charts with relative path
(see https://ceph.github.io/csi-charts/index.yaml).

Artifactory contains support for virtual repositories. Virtual
repositories are able to cache remote repositories and provide
offline access to those repositories.

In order for this to work correctly, charts must specify absolute base URL to
artifacts.

This commit adds this base URL using argument for helm index command.

URL with previous approach:
```yaml
    urls:
    - cephfs/ceph-csi-cephfs-3.13.0.tgz
```

URL with current approach (my personal "lirt" repo was used to test this, the
MR specifies correct "ceph" path)
```yaml
    urls:
    - https://lirt.github.io/csi-charts/cephfs/ceph-csi-cephfs-3.13.0.tgz
```

Signed-off-by: Ondrej Vasko <[email protected]>
@Lirt Lirt force-pushed the compatibility/helm-artifactory branch from 0944d65 to 971a2cd Compare February 5, 2025 08:50
@Lirt
Copy link
Contributor Author

Lirt commented Feb 5, 2025

Hi @nixpanic,

apologize for rookie mistakes :-).

Updated message+body and gpg verification of commit.

@nixpanic
Copy link
Member

nixpanic commented Feb 5, 2025

Hi @nixpanic,

apologize for rookie mistakes :-).

Updated message+body and gpg verification of commit.

No problem, thanks for the update! Some of the automated checks are rather strict, these things happen to all of us 😄

@nixpanic nixpanic requested a review from a team February 5, 2025 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/multi-arch-build skip building on multiple architectures component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants