Bug 1808118: Making logging parameter optional #727
Conversation
Logging has been deprecated in favour of LogLevel, therefore this field is now optional and should so be updated on api accordingly.
openshift-ci-robot
added
do-not-merge/work-in-progress
|
@ricardomaraschini: This pull request references Bugzilla bug 1808118, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ricardomaraschini
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
/assign @dmage As soon as I have your OK on this matter I will assign someone else. |
| @@ -81,6 +81,7 @@ type ImageRegistrySpec struct { | |||
| // replicas determines the number of registry instances to run. | |||
| Replicas int32 `json:"replicas"` | |||
| // logging is deprecated, use logLevel instead. | |||
| // +optional | |||
| Logging int64 `json:"logging"` | |||
There was a problem hiding this comment.
Do we want to have omitempty there?
There was a problem hiding this comment.
Zero is unfortunately still a valid value. BTW, now that LogLevel has a default value it screw up the whole logic and I gonna have to rethink the PR on CIRO.
There was a problem hiding this comment.
defaulting to 2 or whatever corresponds to Normal would be an option too.
There was a problem hiding this comment.
@sttts how defaulting works for upgraded clusters? Will we see the default value after upgrades if the previous version didn't have this field? Or we'll see a zero value?
I'm more curious about logLevel, we expected to see an empty string there.
There was a problem hiding this comment.
We will see the default. It is applied when receiving something from a client and when reading from etcd.
There was a problem hiding this comment.
I don't think this is not an option. We can't know if logging was deliberately set to 0 or it is the default integer value. If set by the user we should use "error" if not then we should be using LogLevel field instead.
There was a problem hiding this comment.
Do we care about logLevels so much? Would ignore the zero here, deprecate and be done with it.
There was a problem hiding this comment.
@sttts why do we need this defaulter? I'd rather remove this defaulter than invent some weird aggregator for these fields.
There was a problem hiding this comment.
We discussed it on today's scrum, we'll use maximum of both
There was a problem hiding this comment.
why do we need this defaulter?
Why do you build non-future proof APIs ? You should never depend on unspecified vs. default value (vs. zero value) of other fields.
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dmage, ricardomaraschini, sttts 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 |
openshift-ci-robot
added
the
approved
|
@ricardomaraschini: Some pull requests linked via external trackers have merged: The following pull requests linked via external trackers have not merged: These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Bugzilla bug 1808118 has not been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Logging has been deprecated in favour of LogLevel, therefore this field
is now optional and should so be updated on api accordingly.