tokers commented on a change in pull request #213: URL: https://github.com/apache/apisix-ingress-controller/pull/213#discussion_r564965301
########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) Review comment: Typo: Apache. ########## File path: charts/apisix-ingress-controller/Chart.yaml ########## @@ -15,13 +15,14 @@ # limitations under the License. # apiVersion: v2 -name: ingress-apisix +name: apisix-ingress-controller description: Apache APISIX Ingress Controller for Kubernetes icon: https://apache.org/logos/res/apisix/apisix.png keywords: - ingress - apisix - nginx + - crd type: application version: 0.1.0 Review comment: Since 0.2.0 was released, we can change the `version` to `0.2.0`. ########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) +- [Helm v3.0+](https://helm.sh/docs/intro/quickstart/#install-helm) + +## Install + +Before installing the chart, you must first install [Apche APISIX](https://github.com/apache/apisix#configure-and-installation). Review comment: Also, you have indicated the dependencies in `Prerequisities`, this statement is redundant. ########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) +- [Helm v3.0+](https://helm.sh/docs/intro/quickstart/#install-helm) + +## Install + +Before installing the chart, you must first install [Apche APISIX](https://github.com/apache/apisix#configure-and-installation). Review comment: Ditto. ########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) +- [Helm v3.0+](https://helm.sh/docs/intro/quickstart/#install-helm) + +## Install + +Before installing the chart, you must first install [Apche APISIX](https://github.com/apache/apisix#configure-and-installation). + +To install the chart with release name `apisix-ingress-controller`: + +```bash +helm install apisix-ingress-controller --namespace apisix . +``` + +## Uninstall + +To uninstall/delete the `apisix-ingress-controller` release: + +```bash +helm uninstall apisix-ingress-controller Review comment: Here you should also add a `--namespace` specifier, keep consistent with the `install`. ########## File path: charts/apisix-ingress-controller/templates/rbac.yaml ########## @@ -157,5 +153,6 @@ roleRef: name: {{ .Release.Name }}-clusterrole subjects: - kind: ServiceAccount - name: {{ .Release.Name }}-serviceaccount + name: {{ include "apisix-ingress-controller.serviceAccountName" . }} Review comment: The name of service account should be opaque to the administrator, i don't think it's necessary to customize it, {{ .Release.Name }}-serviceaccount is enough. ########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) +- [Helm v3.0+](https://helm.sh/docs/intro/quickstart/#install-helm) + +## Install + +Before installing the chart, you must first install [Apche APISIX](https://github.com/apache/apisix#configure-and-installation). + +To install the chart with release name `apisix-ingress-controller`: + +```bash +helm install apisix-ingress-controller --namespace apisix . Review comment: Also, we recommend to use namespace `ingress-apisix`, and it's better to tell users, the target namespace musy be created in advance. ########## File path: charts/apisix-ingress-controller/templates/rbac.yaml ########## @@ -14,11 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. # -apiVersion: v1 -kind: ServiceAccount -metadata: - name: {{ .Release.Name }}-serviceaccount ---- +{{- if .Values.rbac.create }} Review comment: ``{{-`` makes the first line inside the if block joint with the last line, which will cause `apiVersion` commented. You should use `{{` here. ########## File path: charts/apisix-ingress-controller/templates/configmap.yaml ########## @@ -18,24 +18,24 @@ apiVersion: v1 data: config.yaml: | # log options - log_level: {{ .Values.ingressController.config.logLevel | quote }} Review comment: Agree, please revert this change. ########## File path: charts/apisix-ingress-controller/README.md ########## @@ -0,0 +1,44 @@ +<!-- +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +--> + +# Apache APISIX Ingress Controller Helm Chart + +## Prerequisites + +- Kubernetes 1.12+ +- [Apche APISIX](https://github.com/apache/apisix#configure-and-installation) +- [Helm v3.0+](https://helm.sh/docs/intro/quickstart/#install-helm) + +## Install + +Before installing the chart, you must first install [Apche APISIX](https://github.com/apache/apisix#configure-and-installation). + +To install the chart with release name `apisix-ingress-controller`: + +```bash +helm install apisix-ingress-controller --namespace apisix . Review comment: Better to add a `cd ./charts/apisix-ingress-controller` if the directory in `helm install` is `.`. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
