Skip to content

Bug 1808118: Making logging parameter optional #727

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 1 commit into from
Aug 29, 2020
Merged

Bug 1808118: Making logging parameter optional #727

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion imageregistry/v1/00-crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ spec:
description: ImageRegistrySpec defines the specs for the running registry.
type: object
required:
- logging
- managementState
- replicas
properties:
Expand Down
1 change: 1 addition & 0 deletions imageregistry/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
Copy link
Copy Markdown
Contributor

@dmage dmage Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to have omitempty there?

Copy link
Copy Markdown
Contributor Author

@ricardomaraschini ricardomaraschini Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@sttts sttts Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaulting to 2 or whatever corresponds to Normal would be an option too.

Copy link
Copy Markdown
Contributor

@dmage dmage Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Contributor

@sttts sttts Aug 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will see the default. It is applied when receiving something from a client and when reading from etcd.

Copy link
Copy Markdown
Contributor Author

@ricardomaraschini ricardomaraschini Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@sttts sttts Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we care about logLevels so much? Would ignore the zero here, deprecate and be done with it.

Copy link
Copy Markdown
Contributor

@dmage dmage Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts why do we need this defaulter? I'd rather remove this defaulter than invent some weird aggregator for these fields.

Copy link
Copy Markdown
Contributor

@dmage dmage Aug 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it on today's scrum, we'll use maximum of both

Copy link
Copy Markdown
Contributor

@sttts sttts Aug 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// resources defines the resource requests+limits for the registry pod.
// +optional
Expand Down