-
Notifications
You must be signed in to change notification settings - Fork 75
Use K_GCP_AUTH_TYPE to differentiate authentication mode #1947
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: grac3gao 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 |
pkg/reconciler/intevents/pullsubscription/resources/receive_adapter.go
Outdated
Show resolved
Hide resolved
Ready to review. Addressed comments and added more UT for event handler |
pkg/reconciler/intevents/pullsubscription/keda/pullsubscription_test.go
Outdated
Show resolved
Hide resolved
/test pull-google-knative-gcp-wi-tests |
@@ -336,7 +337,7 @@ func TestMarkBrokerCellStatus(t *testing.T) { | |||
wantType apis.ConditionType | |||
wantCondition corev1.ConditionStatus | |||
}{{ | |||
name: "mark ingressReady unknown", | |||
name: fmt.Sprintf("mark %s unknown", string(BrokerCellConditionIngress)), |
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.
I agree with @AlexandraRoatis that we should use the same keyword as the rest of the code, but my personal preference is to do it as a string literal. My reasoning is that if a test fails, it makes it very easy to grep/ctrl-F to find the test case.
Not needed for this PR, but I just don't want this to become the new style :)
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.
This makes sense, I agree that having a string to search when test fail is helpful for the debugging purpose (which I was not aware of before), and I guess this might be one of the reasons why we have names for each sub-UT. I'll change it.
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.
Ok. I see the value in having the searchable string. I have no objection to reverting the changes.
The following is the coverage report on the affected files.
|
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
/hold
Holding in case @AlexandraRoatis has any more comments.
/lgtm |
Fixes #
Proposed Changes
Release Note
Docs