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

refactoring helm chart #213

Merged
merged 2 commits into from
Feb 3, 2021
Merged

refactoring helm chart #213

merged 2 commits into from
Feb 3, 2021

Conversation

junnplus
Copy link
Contributor

@junnplus junnplus commented Jan 26, 2021


New feature or improvement

merge base and ingress-apisix helm chart

@codecov-io
Copy link

codecov-io commented Jan 26, 2021

Codecov Report

Merging #213 (245f897) into master (549d67d) will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #213      +/-   ##
==========================================
+ Coverage   44.52%   44.55%   +0.02%     
==========================================
  Files          32       33       +1     
  Lines        2019     2020       +1     
==========================================
+ Hits          899      900       +1     
  Misses        981      981              
  Partials      139      139              
Impacted Files Coverage Δ
test/e2e/e2e.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 549d67d...245f897. Read the comment docs.

charts/apisix-ingress-controller/Chart.yaml Show resolved Hide resolved
charts/apisix-ingress-controller/README.md Outdated Show resolved Hide resolved
charts/apisix-ingress-controller/README.md Outdated Show resolved Hide resolved
charts/apisix-ingress-controller/README.md Outdated Show resolved Hide resolved
charts/apisix-ingress-controller/README.md Outdated Show resolved Hide resolved
@tokers
Copy link
Contributor

tokers commented Jan 27, 2021

@junnplus The installation of CRDs (and the ClusterRole) should be optional, if we have multiple Ingress controller Deployments, only the first installation should apply them, you can refer to https://github.com/Kong/charts/blob/main/charts/kong/values.yaml#L346 as a reference.

@junnplus
Copy link
Contributor Author

junnplus commented Jan 27, 2021

@tokers installCRDs of kong is for compatibility helm2. this chart require helm 3.0+.

see more in chart best practices

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.

if need to be compatible helm2, I can make additional pr for it.

@tokers
Copy link
Contributor

tokers commented Jan 27, 2021

@tokers installCRDs of kong is for compatibility helm2. this chart require helm 3.0+.

see more in chart best practices

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.

if need to be compatible helm2, I can make additional pr for it.

Sorry, i'm not so familiar with Helm, just want to know how to skip installing CRDs in helm 3.0+.

@junnplus
Copy link
Contributor Author

@tokers Any other questions?

@tokers
Copy link
Contributor

tokers commented Jan 28, 2021

@junnplus LGTM, will need other reviewers to dome double check. Thank you again!

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM, @starsz do you have time to look at this PR?

Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply, glad to see your discussion.

charts/apisix-ingress-controller/Chart.yaml Outdated Show resolved Hide resolved
charts/apisix-ingress-controller/values.yaml Outdated Show resolved Hide resolved
@junnplus
Copy link
Contributor Author

junnplus commented Feb 2, 2021

@gxthrj Can we merge this PR? cc @tokers

@tokers
Copy link
Contributor

tokers commented Feb 2, 2021

@junnplus Yes it can be, the broken e2e case is not related with the changes in this PR. It should be fixed in another PR.

@gxthrj

@tokers
Copy link
Contributor

tokers commented Feb 2, 2021

@junnplus Please update your branch with the newest master, it fixed the broken e2e case, after that, your PR can be merged :)

@junnplus
Copy link
Contributor Author

junnplus commented Feb 2, 2021

ping @tokers

@gxthrj gxthrj merged commit b51bd46 into apache:master Feb 3, 2021
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.

request help: separated base chart or merge base chart
5 participants