Add Helm chart #11
Merged
Add Helm chart #11
+1,124
−108
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: d-kuro <[email protected]>
d-kuro
force-pushed
the
d-kuro/helm
branch
13 times, most recently
from
f6350f5 to
849dfbf
Compare
Signed-off-by: d-kuro <[email protected]>
Signed-off-by: d-kuro <[email protected]>
Signed-off-by: d-kuro <[email protected]>
ymmt2005
requested changes
Aug 24, 2021
| containers: | ||
| - name: manager | ||
| image: "{{ .Values.image.repository }}:{{ default .Chart.AppVersion .Values.image.tag }}" | ||
| {{- with .Values.image.pullPolicy }} |
There was a problem hiding this comment.
What happens when pullPolicy is not set? Is it always set?
If so, can we simplify this as below?
imagePullPolicy: {{ .Values.image.pullPolicy }}There was a problem hiding this comment.
The default is undefined and the imagePullPolicy field is not added if not specified.
accurate/charts/accurate/values.yaml
Line 10 in d5cdc02
I will explicitly set IfNotPresent.
There was a problem hiding this comment.
So, if pullPolicy is not set, this would render as follows, right?
spec:
containers:
- name: manager
image: ...
imagePullPolicy: # emptyI don't like this. Could you omit imagePullPolicy: altogether when pullPolicy is not set?
zoetrope
reviewed
Aug 24, 2021
zoetrope
reviewed
Aug 24, 2021
zoetrope
reviewed
Aug 24, 2021
Signed-off-by: d-kuro <[email protected]>
d-kuro
force-pushed
the
d-kuro/helm
branch
2 times, most recently
from
e6c44a4 to
f7890ab
Compare
Signed-off-by: d-kuro <[email protected]>
|
I fixed the pull request. Review Poinsts: |
ymmt2005
reviewed
Sep 16, 2021
| name: {{ template "accurate.fullname" . }}-additional-resources | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: {{ template "accurate.fullname" . }}-controller-manager |
There was a problem hiding this comment.
Then, users cannot stop creating a ServiceAccount, right?
| containers: | ||
| - name: manager | ||
| image: "{{ .Values.image.repository }}:{{ default .Chart.AppVersion .Values.image.tag }}" | ||
| {{- with .Values.image.pullPolicy }} |
There was a problem hiding this comment.
So, if pullPolicy is not set, this would render as follows, right?
spec:
containers:
- name: manager
image: ...
imagePullPolicy: # emptyI don't like this. Could you omit imagePullPolicy: altogether when pullPolicy is not set?
| {{- with .Values.image.pullPolicy }} | ||
| imagePullPolicy: {{ . }} | ||
| {{- end }} | ||
| {{- with .Values.controller.extraArgs }} |
There was a problem hiding this comment.
So, it renders as follows, right?
args: # emptySame above. Please fix.
zoetrope
reviewed
Sep 17, 2021
zoetrope
reviewed
Sep 17, 2021
zoetrope
reviewed
Sep 17, 2021
Signed-off-by: d-kuro <[email protected]>
Signed-off-by: d-kuro <[email protected]>
Signed-off-by: d-kuro <[email protected]>
Signed-off-by: d-kuro <[email protected]>
Is there a case where a User wants to stop creating a ServiceAccount?
No, if the field is empty the Rendering with the default values will result in the following YAML: <snip>
# Source: accurate/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
name: accurate-controller-manager
namespace: accurate
labels:
app.kubernetes.io/component: controller
helm.sh/chart: accurate-0.1.0
app.kubernetes.io/name: accurate
app.kubernetes.io/version: "0.1.0"
app.kubernetes.io/managed-by: Helm
spec:
replicas: 2
selector:
matchLabels:
app.kubernetes.io/component: controller
template:
metadata:
annotations:
checksum/config: 856d9f37889d4ef4ad9a5bbec38b558ab0e1c5db0dac189d4e9aceadf52be133
labels:
app.kubernetes.io/component: controller
spec:
containers:
- name: manager
image: "ghcr.io/cybozu-go/accurate:0.1.0"
ports:
- containerPort: 9443
name: webhook-server
protocol: TCP
- containerPort: 8081
name: health
protocol: TCP
- containerPort: 8080
name: metrics
protocol: TCP
resources:
requests:
cpu: 100m
memory: 20Mi
securityContext:
allowPrivilegeEscalation: false
readOnlyRootFilesystem: true
env:
- name: POD_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
livenessProbe:
httpGet:
path: /healthz
port: health
initialDelaySeconds: 15
periodSeconds: 20
readinessProbe:
httpGet:
path: /readyz
port: health
initialDelaySeconds: 5
periodSeconds: 10
volumeMounts:
- mountPath: /tmp/k8s-webhook-server/serving-certs
name: cert
readOnly: true
- mountPath: /etc/accurate
name: config
securityContext:
runAsNonRoot: true
serviceAccountName: accurate-controller-manager
terminationGracePeriodSeconds: 10
volumes:
- name: cert
secret:
defaultMode: 420
secretName: webhook-server-cert
- configMap:
name: accurate-config
name: config
<snip> |
I don't think users want to avoid creating a ServiceAccount, but my point is that I'd like either one of the following:
I see. Thank you. |
|
I see, thank you for comments.
In the This is no longer in use and has been removed in the following commit: So a ServiceAccount is always created. |
|
Great! |
Difference from kustomizeTest codespackage tests
import (
"os"
"path/filepath"
"strings"
"testing"
"github.com/d-kuro/helmut"
"github.com/d-kuro/helmut/assert"
"github.com/d-kuro/helmut/util"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
)
func TestHelmCharts(t *testing.T) {
bytes, err := os.ReadFile(filepath.Join("testdata", "kustomize.yaml"))
if err != nil {
t.Fatal(err)
}
const releaseName = "accurate"
r := helmut.New()
manifests, err := r.RenderTemplates(releaseName, filepath.Join("..", "accurate"), helmut.WithNamespace("accurate"), helmut.WithIncludeCRDs())
if err != nil {
t.Fatal(err)
}
splitManifests, err := util.SplitManifests(bytes)
if err != nil {
t.Fatal(err)
}
removeSiffux := func(key helmut.ObjectKey) helmut.ObjectKey {
if strings.HasPrefix(key.Name, "accurate-config") {
key.Name = "accurate-config"
return key
}
return key
}
for _, manifest := range splitManifests {
obj, _ := util.MustRawManifestToObject(clientgoscheme.Scheme, manifest)
key, err := helmut.NewObjectKeyFromObject(obj)
if err != nil {
t.Fatal(err)
}
t.Run(key.String(), func(t *testing.T) {
assert.Contains(t, manifests, obj, assert.WithAdditionalKeys(removeSiffux))
})
}
} |
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
refs: #10
Review Points
TODOs
Diffs