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

Adopt the Helm Chart from the deprecated repository #30

Merged
merged 2 commits into from
Jan 9, 2021

Conversation

yonatankahana
Copy link
Contributor

@yonatankahana yonatankahana commented Dec 20, 2020

What is this PR

The latest official helm chart is in the old and deprecated repository and not currently being maintained. This chart uses very old image and soon (i hope) we will publish newer images and we will need to update the chart accordingly with the new features available (such as #20). To be ready for that we first need to bring it home.

Changes besides just copy and paste the old chart:

Notice to reviewer

This PR consist of 2 commits, the first commit brings the chart AS-IS from the old repository, and the second commit shows my changes since then.
I did this to make the review process easier because most of the files not modified. Once we are ready to merge I will squash them into single commit if required.

Files are as is from deprecated repo
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 20, 2020
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Dec 20, 2020
@petermicuch
Copy link
Contributor

@yonatankahana would it be possible to add helm index directly to this repo, so that people are still able to trigger helm install withouth the need to clone git repo itself? Or is there any plan to provide "add repo" support?

@yonatankahana
Copy link
Contributor Author

@yonatankahana would it be possible to add helm index directly to this repo, so that people are still able to trigger helm install withouth the need to clone git repo itself? Or is there any plan to provide "add repo" support?

Yes, It it possible but not necessarily the best option, especially in the scope of this PR. My idea was to first bring it here and then, in a separate PR, we can automate the release process and consider where to host the packaged chart.
So the installation with git clone is not the final form of installation, but an intermediate stage.

@pierluigilenoci
Copy link

@wongma7 @jsafrane @kmova @jackielii @ashishranjan738 any of you have time to take a look? Thanks! ❤️

@immanuelfodor
Copy link

Would the migration from nfs-client-provisioner be as simple as remove the old and in with the new?

@yonatankahana
Copy link
Contributor Author

yonatankahana commented Jan 6, 2021

Would the migration from nfs-client-provisioner be as simple as remove the old and in with the new?

yes.

@wongma7
Copy link
Contributor

wongma7 commented Jan 6, 2021

Yes, It it possible but not necessarily the best option, especially in the scope of this PR. My idea was to first bring it here and then, in a separate PR, we can automate the release process and consider where to host the packaged chart.
So the installation with git clone is not the final form of installation, but an intermediate stage.

I agree with this approach. In future it should be automated with github actions, for now at least the chart has to be imported into the repo.

@wongma7
Copy link
Contributor

wongma7 commented Jan 7, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 7, 2021
@pierluigilenoci
Copy link

pierluigilenoci commented Jan 8, 2021

@jsafrane @kmova could you please approve?

- name: nfs-subdir-external-provisioner-root
{{- if .Values.buildMode }}
emptyDir: {}
{{- else if .Values.nfs.mountOptions }}
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@kmova
Copy link
Contributor

kmova commented Jan 9, 2021

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kmova, yonatankahana

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

The pull request process is described here

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit ba33603 into kubernetes-sigs:master Jan 9, 2021
humblec pushed a commit to humblec/nfs-subdir-external-provisioner that referenced this pull request May 11, 2022
Adopt the Helm Chart from the deprecated repository
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants