Add support for HA control plane #337
Conversation
| @@ -119,6 +119,15 @@ spec: | |||
| name: | |||
| description: The name of the ConfigMap or Secret | |||
| type: string | |||
| highAvailability: | |||
There was a problem hiding this comment.
We're mixing case convention here, should be high-availability to be consistent
| if instance.Spec.HighAvailability == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
You could put this outside the closure for a skosh more performance. Transform will ignore a nil Transformer
There was a problem hiding this comment.
I would rather do this in a follow-up that changes everywhere that does this, since this PR is blocking the release
| if cm.Data == nil { | ||
| cm.Data = map[string]string{} | ||
| } | ||
| cm.Data[enabledComponentsKey] = componentsValue |
There was a problem hiding this comment.
You might want to log a warning if this value is also present in the instance.Config map... maybe?
| if err := scheme.Scheme.Convert(cm, u, nil); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
| if err := scheme.Scheme.Convert(cm, u, nil); err != nil { | |
| return err | |
| } | |
| return scheme.Scheme.Convert(cm, u, nil) |
| if err := scheme.Scheme.Convert(deployment, u, nil); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
| if err := scheme.Scheme.Convert(deployment, u, nil); err != nil { | |
| return err | |
| } | |
| return scheme.Scheme.Convert(deployment, u, nil) |
| }, | ||
| } | ||
|
|
||
| for i, _ := range cases { |
There was a problem hiding this comment.
Format Go code:
| for i, _ := range cases { | |
| for i := range cases { |
| for _, fn := range platforms { | ||
| for i := range platforms { | ||
| fn := platforms[i] |
There was a problem hiding this comment.
Is this really necessary?
|
I think you've accidentally discovered a ginormous manifestival bug. Holding until I create a fix... |
|
/hold |
|
/hold cancel |
| if err := unstructured.SetNestedStringMap(u.Object, data, "data"); err != nil { | ||
| return err | ||
| } | ||
| log.Infof("Updated configmap/config-leader-election: %v", u.Object) |
There was a problem hiding this comment.
This is just noise. I would delete these logs since manifestival will log the changes from them anyway. But if you insist, make 'em debug?
There was a problem hiding this comment.
It actually won't give you the full content of the object, which i needed, but I can make a PR for manifestival for that.
|
The following is the coverage report on the affected files.
|
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: houshengbo, pmorie 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 |
|
Part of knative/pkg#1007 |
Fixes #320
Proposed Changes
KnativeServing.spec.highAvailabilityRelease Note