Fix imports for sub test.

[n3wscott]

Fixes #1068

Proposed Changes

• inline the reconciler test helpers.

Release Note

NONE

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [n3wscott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[n3wscott]

/assign @grantr

I think my IDE did this for me when I moved the file out of v1alpha1.

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/subscription/subscription.go	Do not exist	80.6%

[n3wscott]

/test pull-knative-eventing-integration-tests

[Harwayne]

Remove this newline.

[n3wscott]

working as intended. I have been separating the . imports from the others.

[Harwayne]

What is a ". import"?

[n3wscott]

what you are commenting on: . "github.com/knative/eventing/pkg/reconciler/testing"

[Harwayne]

So ". import" is one without an explicit alias? E.g. "github.com/foo/bar" as opposed to "qux github.com/foo/bar"?

[n3wscott]

Also you can do this: import _ "github.com/foo/goo" and it will not import anything but run the init func and maybe that mutates global state.

[Harwayne]

The project should have some well defined standard ordering. The one I have been using is standard libraries -> new line -> all others. I can see the advantage of moving ". imports" and possibly "_ imports" into separate structures, do we want the k8s stuff separate too? If we do that, then do we want the knative stuff separate as well?

[n3wscott]

Most go projects will separate in the following ways:

• system stuff
• blocks of dep imports that are from the same host *n
• project stuff

[n3wscott]

serving tends to do this, k8s.io gets its own block: https://github.com/knative/serving/blob/master/cmd/controller/main.go#L23

[grantr]

I think dot imports should be separated from other imports, since the reader must know about them to read the code. I don't have strong opinions on import ordering otherwise. gofmt already enforces the block of standard lib, so that's settled. I could understand having separate blocks for vendored code versus in-repo code.

IMO dot imports reduce readability of the code by adding magic. I'd prefer that these not be used, but I won't block the PR for them. Correct me if I'm wrong @n3wscott, but I believe they're used here to work around the fact that when functional options are used, each functional option requires a reference to its package. Without dot imports this would be verbose.

fixtures.NewSubscription(subscriptionName, testNS,
  fixtures.WithSubscriptionChannel(channelGVK, channelName),
  fixtures.WithSubscriptionSubscriberRef(subscriberGVK, subscriberName),
)

I prefer builders for specifying fixtures. Builders require only one package reference. When using a builder, my IDE can offer concise suggestions of which builder methods can be called next. With functional options, this is more difficult because any function in scope could conceivably be called (I'm not sure if any IDE is sophisticated enough to filter only the functions with the correct signature).

fixtures.NewSubscription(subscriptionName, testNS).
  WithSubscriptionChannel(channelGVK, channelName).
  WithSubscriptionSubscriberRef(subscriberGVK, subscriberName)

Builders have the disadvantage of not returning a concrete struct of their built type, but this can be worked around by embedding the built struct into the builder struct.

Functional options have the advantage of allowing certain options to act on any runtime.Object (as pointed out by @n3wscott), which is cool, but seems even harder to document.

[grantr]

/lgtm

/hold
Holding since @Harwayne asked for an answer on import block style.

@Harwayne What do you think about this convention?

import (
  // standard lib, enforced by gofmt
  "log"

  // vendored code. These packages cannot be changed by the developer.
  corev1 "k8s.io/api/core/v1"

  // in-repo code. These packages can be changed by the developer.
  "github.com/knative/eventing/pkg/broker"

  // dot imports. These are separate because the developer must recognize them in advance
  // to understand the code.
  . "github.com/knative/pkg/controller/testing"
)

Perhaps an interesting side project for when someone has a free day to tinker with go reformatting...

[n3wscott]

I think it vendored code gets grouped by host:

import (
  // standard lib, enforced by gofmt
  "log"

  // vendored code. These packages cannot be changed by the developer.
  corev1 "k8s.io/api/core/v1"

  // vendored code. These packages cannot be changed by the developer.
  cloudevents "github.com/cloudevents/sdk-go"

  // in-repo code. These packages can be changed by the developer.
  "github.com/knative/eventing/pkg/broker"

  // dot imports. These are separate because the developer must recognize them in advance
  // to understand the code.
  . "github.com/knative/pkg/controller/testing"
)

[Harwayne]

@grantr

I'm lazy, so my preference is to do as little work as possible:

import (
  // standard lib, enforced by gofmt
  "log"

  // dot imports. These are separate because the developer must recognize them in advance
  // to understand the code. These are also quite rare.
  . "github.com/knative/pkg/controller/testing"

  // All 'normal' imports that aren't part of the standard lib, regardless of whether they are in repo or vendored.
  corev1 "k8s.io/api/core/v1"
  "github.com/knative/eventing/pkg/broker"
)

But at the end of the day, I just want to not have to think about this stuff, both when writing and reviewing. So whatever an automated tool does is fine for me.

[grantr]

Opened #1082 to track the imports discussion. Now that that's separated, I think this PR can be unheld.

/hold cancel