Move operator to gotohelm #1509
Conversation
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
3 times, most recently
from
2d415d6
to
c5a40a2
Compare
RafalKorepta
requested review from
JakeSCahill,
chrisseto and
andrewstucki
as code owners
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
5 times, most recently
from
0e673f9
to
cf1477c
Compare
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
5 times, most recently
from
9dfd2f8
to
76dc85a
Compare
| require.NoError(t, os.WriteFile("testdata/template-cases-generated.txtar", archive, 0o644)) | ||
| } | ||
|
|
||
| func makeSureTagIsNotEmptyString(values PartialValues, fuzzer *fuzz.Fuzzer) { |
There was a problem hiding this comment.
Should this check be enforced by the chart's JSON schema?
There was a problem hiding this comment.
It is exactly JSON schema that enforces me to re-generate any tag. That's why this function is created.
There was a problem hiding this comment.
Hmmm, shouldn't this block handle that for you in that case? Or do we never end up generating a valid values due to the number of fields here?
There was a problem hiding this comment.
It should handle that, but for me it was to much false positive. I didn't want to wait.
| ) | ||
| } | ||
|
|
||
| if overrides.Spec.TopologySpreadConstraints != nil && len(overrides.Spec.TopologySpreadConstraints) > 0 { |
There was a problem hiding this comment.
I don't think it was necessary to fully implement this when we just need to be able to override SecurityPolicy 😅
Did you try just calling merge on the pod specs and then filling in fields that need special handling? I think I tried that for containers and it didn't work but I don't recall why....
There was a problem hiding this comment.
It was not reliable, that's why I copied from redpanda chart and filled all other fields.
There was a problem hiding this comment.
merge wasn't reliable or the implementation in the redpanda chart wasn't reliable?
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| ) | ||
|
|
||
| func ClusterRole(dot *helmette.Dot) []rbacv1.ClusterRole { |
There was a problem hiding this comment.
Note for the future (not this PR). We could add the ability to load files into gotohelm and just read these from disk so they don't get out of sync. Similar to what I did here: #1285
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
from
76dc85a
to
1b9bf71
Compare
Any array that has null assigned would fail the helm template with invalid schema.
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
2 times, most recently
from
31220b5
to
29a41e6
Compare
| require.NoError(t, os.WriteFile("testdata/template-cases-generated.txtar", archive, 0o644)) | ||
| } | ||
|
|
||
| func makeSureTagIsNotEmptyString(values PartialValues, fuzzer *fuzz.Fuzzer) { |
There was a problem hiding this comment.
Hmmm, shouldn't this block handle that for you in that case? Or do we never end up generating a valid values due to the number of fields here?
| ) | ||
| } | ||
|
|
||
| if overrides.Spec.TopologySpreadConstraints != nil && len(overrides.Spec.TopologySpreadConstraints) > 0 { |
There was a problem hiding this comment.
merge wasn't reliable or the implementation in the redpanda chart wasn't reliable?
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
from
29a41e6
to
82baddd
Compare
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
8 times, most recently
from
2034b23
to
927d09f
Compare
The helmignore is required if testdata consist of large generated "golden" files like in current operator chart. Helm cli would take that files and include in the release. Currently console 0.7.29 and connectors 0.1.12 have testdata directy included. With this commit they will omit go files and testdata.
RafalKorepta
force-pushed
the
rk/k8s-316/security-context
branch
from
927d09f
to
7f3b247
Compare
To solve problem of missing security context this commit moves almost all resources to go. genschema is currently broken as I tried to solve particular issue with operator schema generation. Need to fix before merging this PR.