Refactor k8s source to use the sdk. #123
Conversation
knative-prow-robot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: n3wscott If they are not already assigned, you can assign the PR to them by writing 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 |
knative-prow-robot
added
the
size/L
knative-prow-robot
added
size/XL
knative-prow-robot
added
size/XXL
n3wscott
changed the title
knative-prow-robot
removed
the
do-not-merge/work-in-progress
|
/test pull-knative-eventing-sources-go-coverage |
|
/assign @grantr |
|
The following is the coverage report on pkg/.
|
| kubernetesEventSourceCondSet.Manage(s).MarkTrue(KubernetesEventSourceConditionReady) | ||
| // PropagateContainerSourceStatus examines the given container source and synchronizes the conditions that matter to | ||
| // the kubernetes event source status. | ||
| func (ss *KubernetesEventSourceStatus) MarkContainerSourceReadyStatus(cs ContainerSourceStatus) { |
There was a problem hiding this comment.
The comment starts with PropagateContainerSourceStatus, which seems like what this method does, but the method is named MarkContainerSourceReadyStatus.
| kubernetesEventSourceCondSet.Manage(s).MarkTrue(KubernetesEventSourceConditionReady) | ||
| // PropagateContainerSourceStatus examines the given container source and synchronizes the conditions that matter to | ||
| // the kubernetes event source status. | ||
| func (ss *KubernetesEventSourceStatus) MarkContainerSourceReadyStatus(cs ContainerSourceStatus) { |
There was a problem hiding this comment.
If we expect people to use ContainerSource this way (having their own source wrap it), this method should be moved somewhere it can be shared.
func PropagateContainerSourceStatus(cm ConditionManager, cs ContainerSourceStatus)Then in this file:
func PropagateContainerSourceStatus(cs ContainerSourceStatus) {
containersource.PropagateConatinerSourceStatus(kubernetesEventSourceCondSet.Manage(s), cs)
}| kubernetesEventSourceCondSet.Manage(s).MarkTrue(KubernetesEventSourceConditionReady) | ||
| // PropagateContainerSourceStatus examines the given container source and synchronizes the conditions that matter to | ||
| // the kubernetes event source status. | ||
| func (ss *KubernetesEventSourceStatus) MarkContainerSourceReadyStatus(cs ContainerSourceStatus) { |
There was a problem hiding this comment.
ss -> s to match the other functions in the file.
|
|
||
| reason := "" | ||
| message := "" | ||
| isFalse := false |
There was a problem hiding this comment.
How about statusFalse? For some reason isFalse feels wrong to me.
| // the source does not have Ready status True. | ||
| func (s *KubernetesEventSourceStatus) MarkUnready(reason, messageFormat string, messageA ...interface{}) { | ||
| kubernetesEventSourceCondSet.Manage(s).MarkFalse(KubernetesEventSourceConditionReady, reason, messageFormat, messageA...) | ||
| func appendMessage(a string, b string) string { |
There was a problem hiding this comment.
How about using strings.Join instead?
Above would change to something like:
messages := make([]string)
...
messages = append(messages, "SinkProvided status is nil")
...
MarkUnknown(..., reason, strings.Join(messages, "; "))| }, | ||
| } | ||
|
|
||
| func TestAllCases(t *testing.T) { |
| duckAddKnownTypes(scheme.Scheme) | ||
| } | ||
|
|
||
| var testCases = []controllertesting.TestCase{ |
There was a problem hiding this comment.
Move this variable inside the TestAllCases method.
| Reconciles: &sourcesv1alpha1.KubernetesEventSource{}, | ||
| InitialState: []runtime.Object{ | ||
| func() runtime.Object { | ||
| s := getSource() |
There was a problem hiding this comment.
Every call to getSource() (except one where I don't think it matters), is immediately followed by s.UID = sourceUID, move that into the getSource() method.
| return u | ||
| }(), | ||
| }, | ||
| ReconcileKey: fmt.Sprintf("%s/%s", testNS, sourceName), |
There was a problem hiding this comment.
I recommend removing ReconcileKey and IgnoreTimes out of every test case and just set them in the test itself, because every test case uses the same values for those two fields.
| // Add creates a new KubernetesEventSource Controller and adds it to the Manager | ||
| // with default RBAC. The Manager will set fields on the Controller and Start it | ||
| // when the Manager is Started. | ||
| // Add creates a new GitHubSource Controller and adds it to the |
|
@n3wscott what's the status here ? |
|
/assign Harwayne |
Proposed Changes
Release Note