DeploymentConfig replicas should be optional, other fields too #17035
Conversation
openshift-ci-robot
added
the
size/L
openshift-merge-robot
added
approved
|
@openshift/sig-master |
smarterclayton
added
kind/bug
|
imageapi changes lgtm. |
pkg/apps/apis/apps/v1/types.go
Outdated
| Triggers DeploymentTriggerPolicies `json:"triggers" protobuf:"bytes,2,rep,name=triggers"` | ||
|
|
||
| // Replicas is the number of desired replicas. | ||
| Replicas int32 `json:"replicas" protobuf:"varint,3,opt,name=replicas"` | ||
| // +optional | ||
| Replicas *int32 `json:"replicas" protobuf:"varint,3,opt,name=replicas"` |
There was a problem hiding this comment.
@smarterclayton how is this not a breaking change?
There was a problem hiding this comment.
The value still defaults. No client ever sees this being null. You can make a required field not required as long as you provide a default.
There was a problem hiding this comment.
It will break everyone using our Go client :/
But I guess on the pure API front the serialization will go fine when we default unless client is able to see the object before defaulting happens (say with GetOptions.IncludeUninitialized) but I think we initialize before storing the object.
There was a problem hiding this comment.
it will break everyone using our Go client :/
we don't have a supported go client yet.
But I guess on the pure API front the serialization will go fine when we default
yes
unless client is able to see the object before defaulting happens
it cannot
There was a problem hiding this comment.
we don't have a supported go client yet.
so we don't "support" client/clientsets in origin codebase? because people use that...
There was a problem hiding this comment.
We don't guarantee they don't change. Users have to track updates to code.
smarterclayton
force-pushed
the
set_defaults
branch
from
5ec1b35
to
9cfe5b4
Compare
|
@liggitt can you sanity check this? trying to be consistent with upstream, i'm fairly certain this is safe but want the double check |
| @@ -24,6 +26,7 @@ func TestDefaults(t *testing.T) { | |||
| original: &DeploymentConfig{}, | |||
| expected: &DeploymentConfig{ | |||
| Spec: DeploymentConfigSpec{ | |||
| Replicas: &one, | |||
There was a problem hiding this comment.
doesn't this mean the default changed from 0 to 1?
There was a problem hiding this comment.
Yeah, I suppose there may be people who rely on that. Something we should have fixed during 3.6, but forgot. Going to back this out.
| @@ -33,12 +33,13 @@ type DeploymentConfig struct { | |||
| Spec DeploymentConfigSpec `json:"spec" protobuf:"bytes,2,opt,name=spec"` | |||
|
|
|||
| // Status represents the current deployment state. | |||
| Status DeploymentConfigStatus `json:"status" protobuf:"bytes,3,opt,name=status"` | |||
| Status DeploymentConfigStatus `json:"status,omitempty" protobuf:"bytes,3,opt,name=status"` | |||
There was a problem hiding this comment.
doesn't do anything on a struct. does that matter?
There was a problem hiding this comment.
No, just using it as the canonical optional and "eventually someone will fix go json to not suck"
Make a set of fields truly optional in openapi, and also make replicas a pointer so we can default it.
smarterclayton
force-pushed
the
set_defaults
branch
from
9cfe5b4
to
f5624be
Compare
openshift-ci-robot
added
size/M
|
/retest |
|
Any other comments? /kind bug |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, smarterclayton The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
|
Automatic merge from submit-queue. |
Make a set of fields truly optional in openapi, and also make replicas a
pointer so we can default it.
Found this when I tried to use --validate - these were a bunch of obvious things.