-
Notifications
You must be signed in to change notification settings - Fork 82
Allow consolidated channel to use distributed-style secret format #680
Allow consolidated channel to use distributed-style secret format #680
Conversation
Codecov Report
@@ Coverage Diff @@
## main #680 +/- ##
==========================================
- Coverage 75.33% 75.30% -0.03%
==========================================
Files 132 132
Lines 5782 5775 -7
==========================================
- Hits 4356 4349 -7
Misses 1206 1206
Partials 220 220
Continue to review full report at Codecov.
|
/assign @aliok |
/test pull-knative-sandbox-eventing-kafka-integration-test-channel-consolidated-sasl |
LGTM I can have another look when the failing test got fixed. Thanks for adding the backwards compatibility. We should remember to deprecate the old format in a few releases. |
/hold |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - couple minor nits... I'll leave it to the consolidated channel experts to merge since it's mostly adjusting their implementation.
/assign @matzew
I will take a look at this on Friday (off tomorrow)
On Wed 2. Jun 2021 at 20:20, eric-sap ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/common/config/util.go
<#680 (comment)>
:
> secrets := kubeclient.Get(ctx).CoreV1().Secrets(secretNamespace)
secret, err := secrets.Get(ctx, secretName, metav1.GetOptions{})
if err != nil {
- return nil, err
+ // Consolidated channel backwards-compatibility - A nonexistent secret returns a nil config
+ return nil
That's true, especially since it now applies to the distributed channel as
well. I don't think there would be any users who rely on a missing secret
to cause an error (but I've been wrong about those kinds of things before).
I made the comment more purpose-oriented.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#680 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABGPTXZJV2DA6A5U4LOUZTTQZY6JANCNFSM455QUBYQ>
.
--
Sent from Gmail Mobile
|
/test all |
@travis-minke-sap @eric-sap we need to change the docs repo with this too . I am not holding the PR for that. But please let's get some update there |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
/test all
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eric-sap, matzew 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 |
The following is the coverage report on the affected files.
|
/unhold |
Fixes #679
Proposed Changes
Release Note