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

Add helm chart #2208

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Conversation

stevehipwell
Copy link
Contributor

Description

Adds a Helm chart based on the official manifests in the repo; the Helm repo will be hosted by GitHub pages at https://kubernetes-sigs.github.io/external-dns/.

Adds required GitHub Actions CI.

  • A PR making changes to files in /charts/external-dns/** will trigger the lint and test action
    • If the chart has changed then the chart version should be updated accordingly or else the CI will error
  • If changes are made to files in /charts/external-dns/** on the master branch a release will be made
    • The gh-pages branch needs to be setup so the release action can use it as the Helm repo

The chart is fully documented but I've not linked it in to any of the other documentation as maybe this should be a soft launch? Otherwise could someone point me in the right direction to put the documentation in the correct place.

Fixes #1941

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 9, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @stevehipwell!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 9, 2021
@stevehipwell stevehipwell mentioned this pull request Aug 9, 2021
@stevehipwell
Copy link
Contributor Author

/assign @Raffo

Copy link

@dirien dirien left a comment

Choose a reason for hiding this comment

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

Hi,

Left some comments.

charts/external-dns/Chart.yaml Outdated Show resolved Hide resolved
charts/external-dns/README.md Show resolved Hide resolved
@stevehipwell stevehipwell force-pushed the add-helm-chart branch 2 times, most recently from 742ca7f to 6a95d39 Compare August 10, 2021 08:46
@njuettner
Copy link
Member

Seems like the linter is failing 😞

Events:
  Type     Reason            Age                 From               Message
  ----     ------            ----                ----               -------
  Warning  FailedScheduling  33s (x5 over 5m1s)  default-scheduler  0/1 nodes are available: 1 node(s) had taint {node.kubernetes.io/not-ready: }, that the pod didn't tolerate.

@stevehipwell
Copy link
Contributor Author

@njuettner the Kind action failed (see helm/kind-action#41 for why it didn't error) which causes later actions to fail. If you have the permissions could you re-run the action?

Creating cluster "chart-testing" ...
• Ensuring node image (kindest/node:v1.19.1) 🖼 ...
✓ Ensuring node image (kindest/node:v1.19.1) 🖼
• Preparing nodes 📦 ...
✓ Preparing nodes 📦
• Writing configuration 📜 ...
✓ Writing configuration 📜
• Starting control-plane 🕹️ ...
✓ Starting control-plane 🕹️
• Installing CNI 🔌 ...
✓ Installing CNI 🔌
• Installing StorageClass 💾 ...
✓ Installing StorageClass 💾
• Waiting ≤ 1m0s for control-plane = Ready ⏳ ...
✗ Waiting ≤ 1m0s for control-plane = Ready ⏳
• WARNING: Timed out waiting for Ready ⚠️

@stevehipwell
Copy link
Contributor Author

@Raffo could you re-run the lint-test action for the above mentioned reason?

@stevehipwell
Copy link
Contributor Author

@Raffo @njuettner I've pushed again so it looks like the actions will need to be enabled again.

@Raffo
Copy link
Contributor

Raffo commented Aug 13, 2021

@stevehipwell I enabled CI, the linter is still unhappy.

@stevehipwell
Copy link
Contributor Author

@Raffo I can't get to the logs but was the Kind cluster creation good or did that never get healthy?

@stevehipwell
Copy link
Contributor Author

I got to the logs, it looks like it was Kind. Could you re run the action?

@Raffo
Copy link
Contributor

Raffo commented Aug 13, 2021

@stevehipwell kind being unkind uh 😅 rerunning!

@Raffo
Copy link
Contributor

Raffo commented Aug 13, 2021

And it failed again. I think the build deserves a deeper look on why this is happening.

@jordiprats
Copy link

Hi guys,
I'm hoping this would merged so I took a look, it says:

https://github.com/kubernetes-sigs/external-dns/pull/2208/checks?check_run_id=3320498211

0/1 nodes are available: 1 node(s) had taint {node.kubernetes.io/not-ready: }, that the pod didn't tolerate

I'm not familiar with the cluster were you are running this but sounds like it is spawning a new node to run this pod and you are not waiting long enough for the node to become ready.

@stevehipwell
Copy link
Contributor Author

If I remember correctly it's possible to increase the timeout on the Kind action. I run a number of charts with this exact config and up until recently hadn't seen this issue; up until this point I think I've only ever seen it twice in well over 50 runs.

@k0da
Copy link
Contributor

k0da commented Aug 13, 2021

@stevehipwell you can use act to simulate GHA runs locally. Also we found k3d project is more lightweight to spawn k8s clusters for CI. Here is the action for k3d. JFYI

@stevehipwell
Copy link
Contributor Author

stevehipwell commented Aug 13, 2021

Thanks @k0da, as I said above this pattern has been working for years and hundreds of chart PRs so I've no idea why it's suddenly got brittle.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
@stevehipwell
Copy link
Contributor Author

@Raffo I've updated the Kind action version (I thought I'd already done this) and added a longer wait for the cluster to be ready which should solve this.

Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

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

@stevehipwell CI is green and the chart content looks all right to me. However, I think that we need to remove/change the part on releasing the chart: we have a consolidated release process described in https://github.com/kubernetes-sigs/external-dns/blob/master/docs/release.md and we would need to make creating the release part of it. I also would love to avoid having multiple github releases for what are code and helm changes. Would you be able to propose a change to incorporate that?

@stevehipwell
Copy link
Contributor Author

@Raffo unfortunately the chart releaser will create a GitHub release for each chart release and there is no way to change this behaviour. You'll see in the release action that the suffix -helm-chart is added to distinguish the release.

Would it be easier to merge this PR and add the documentation separately as the first release only requires the gh-pages branch to be created with GitHub pages enabled and the PR to be accepted?

@onedr0p
Copy link
Contributor

onedr0p commented Aug 16, 2021

@stevehipwell would it be beneficial to include the external-dns DNSEndpoint CRD in a crds folder? See the this chart for an example. Otherwise I am not seeing how to install the DNSEndpoint CRD besides k apply -f docs/contributing/crd-source/crd-manifest.yaml

It appears like bitnami includes the CRD in their helm chart. However they are doing it wrong IMO, it should not be templated.

According to the helm docs:

With the arrival of Helm 3, we removed the old crd-install hooks for a more simple methodology. There is now a special directory called crds that you can create in your chart to hold your CRDs. These CRDs are not templated, but will be installed by default when running a helm install for the chart. If the CRD already exists, it will be skipped with a warning. If you wish to skip the CRD installation step, you can pass the --skip-crds flag.

@stevehipwell
Copy link
Contributor Author

@onedr0p if the CRDs are fully supported then they should be in the CRDs chart folder, but as they're not in the standard manifest install I've not added them.

However if you read the Helm 3 docs you will see that although CRD support is partially implemented you shouldn't use it and disable installing CRDs with the --skip-crds flag then manage your CRDs with kubectl.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 18, 2021
@Raffo
Copy link
Contributor

Raffo commented Aug 18, 2021

Would it be easier to merge this PR and add the documentation separately as the first release only requires the gh-pages branch to be created with GitHub pages enabled and the PR to be accepted?

This sounds good to me.

@Raffo
Copy link
Contributor

Raffo commented Aug 18, 2021

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njuettner, Raffo, stevehipwell

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 merged commit 2f1aff4 into kubernetes-sigs:master Aug 18, 2021
@stevehipwell stevehipwell deleted the add-helm-chart branch August 18, 2021 08:18
litewhatever added a commit to litewhatever/external-dns that referenced this pull request Aug 26, 2021
@stevehipwell
Copy link
Contributor Author

@Raffo @njuettner would either of you be willing to sponsor me as a member of the Kubernetes SIGs GitHub organisation?

@Raffo
Copy link
Contributor

Raffo commented Oct 7, 2021

@stevehipwell what would be the need? Approving the Helm chat prs?

@stevehipwell
Copy link
Contributor Author

@Raffo being in the owners file to get notified of chart PRs to review would be the main reason. I maintain multiple Kubernetes SIGs charts so it'd be easier if I was a member.

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.

Official Helm chart
8 participants