Log a warning if the channel doesn't match the default #1178
Conversation
|
@tpantelis is there a fake logger we could use to test this? |
| @@ -214,6 +214,10 @@ func applySubmarinerConfig(brokerInfo *SubmarinerBrokerInfo, submarinerConfig *c | |||
| setIfValueNotDefault(&brokerInfo.CatalogStartingCSV, submarinerConfig.Spec.SubscriptionConfig.StartingCSV) | |||
| setIfValueNotDefault(&brokerInfo.InstallPlanApproval, submarinerConfig.Spec.SubscriptionConfig.InstallPlanApproval) | |||
|
|
|||
| if brokerInfo.CatalogChannel != defaultCatalogChannel { | |||
| logger.Warningf("Tracking non-default catalog channel %q (default is %q)", brokerInfo.CatalogChannel, defaultCatalogChannel) | |||
There was a problem hiding this comment.
Why is this a warning? Is this something a user isn't supposed to do?
There was a problem hiding this comment.
In most cases they shouldn’t, and in all cases support needs to know about it. It could be an info-level message but then it wouldn’t necessarily be logged...
There was a problem hiding this comment.
If you use logger.V(...).Infof it may not be but if you use logger.Infof, it should always be logged.
Not that I know of. But I don't think we should verify specific log messages in unit tests. If the visibility of this message is significant enough, perhaps you should use the event |
skitt
force-pushed
the
warn-on-non-default-channel
branch
from
9d36a38
to
9d46eaa
Compare
|
I just wanted to verify that we’re logging something if the channel is configured to a value other than the default. I don’t think this warrants recording an actual event. |
| @@ -214,6 +214,10 @@ func applySubmarinerConfig(brokerInfo *SubmarinerBrokerInfo, submarinerConfig *c | |||
| setIfValueNotDefault(&brokerInfo.CatalogStartingCSV, submarinerConfig.Spec.SubscriptionConfig.StartingCSV) | |||
| setIfValueNotDefault(&brokerInfo.InstallPlanApproval, submarinerConfig.Spec.SubscriptionConfig.InstallPlanApproval) | |||
|
|
|||
| if brokerInfo.CatalogChannel != defaultCatalogChannel { | |||
| logger.Infof("Tracking non-default catalog channel %q (default is %q)", brokerInfo.CatalogChannel, defaultCatalogChannel) | |||
There was a problem hiding this comment.
This will get logged on every reconcile loop which might get excessive.
Why can't we have support inspect the SubmarinerConfig resource or the Subscription to see if the channel is custom rather than via a log message?
There was a problem hiding this comment.
Hah I was just checking how often the loop runs...
When we discussed this a while back we decided to log a message for two reasons:
- end-users might notice it if things go wrong, and figure out they need to fix something on their own (assuming they remember they set up a specific channel)
- the log message can provide all the information support needs — both the configured value, and the expected default
I’ll memoize the channel value and only log it the first time it changes.
Users can configure a non-default channel, but support needs to know about this, so log it. Signed-off-by: Stephen Kitt <skitt@redhat.com>
skitt
force-pushed
the
warn-on-non-default-channel
branch
from
9d46eaa
to
df494bc
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tpantelis 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 |
|
Kudos, SonarCloud Quality Gate passed! |
|
/cherrypick release-2.8 |
|
@skitt: new pull request created: #1180 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. |
|
/cherrypick release-2.7 |
|
@skitt: new pull request created: #1181 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. |
|
/cherrypick release-2.6 |
|
@skitt: new pull request created: #1182 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. |
Users can configure a non-default channel, but support needs to know about this, so log it.