Avoid importing apis/duck from apis/duck/<<version>> #1388

alvaroaleman · 2020-06-07T02:05:57Z

Moves some type definitions from apis/duck into apis/duck/ducktypes
Creates aliases from apis/duck to the new location to stay compatible
Moves a set of var definitions from the apis/duck/{v1beta1,v1} into apis/duck

knative-prow-robot · 2020-06-07T02:06:09Z

Welcome @alvaroaleman! It looks like this is your first PR to knative/pkg 🎉

knative-prow-robot · 2020-06-07T02:06:10Z

Hi @alvaroaleman. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

alvaroaleman · 2020-06-07T02:06:27Z

/assign @mattmoor

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

alvaroaleman · 2020-06-07T02:11:29Z

apis/duck/v1_tests.go

+
+	"knative.dev/pkg/apis/duck/v1"


This would break compilation

This is the output of goimports, you need:

Suggested change

"knative.dev/pkg/apis/duck/v1"

v1 "knative.dev/pkg/apis/duck/v1"

I have no idea what you are talking about, the change you are suggesting looks like you have some broken tooling. Aliasing an import with its own name is useless mental overhead.

alvaroaleman · 2020-06-07T02:11:45Z

apis/duck/v1beta1/addressable_types.go

-	"knative.dev/pkg/apis/duck"
-	v1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/apis/duck/ducktypes"
+	"knative.dev/pkg/apis/duck/v1"


This would break compilation

why would it?

Because it removes an import that is being referenced?

I guess we need to call it duckv1 to conform to our repository conventions.
I think at some point it saw both named and unnamed import.

there is already a duckv1. My PR does not change anything about the existing imports other than removing the useless v1 prefix.

alvaroaleman · 2020-06-07T02:10:49Z

@mattmoor sorry, I don't understand your comments. According to your suggestions I should remove imports which would break the code.

The code in this PR is properly formatted via go fmt -s:

$ gofmt -s -w ./apis/..
alvaro@t470s-[2020-06-06-22:09]:[try1][1].../tmp.BhWuy7JwT9/pkg
$ git status|grep -v vendor
On branch try1
Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)

no changes added to commit (use "git add" and/or "git commit -a"

alvaroaleman · 2020-06-07T02:11:29Z

apis/duck/v1_tests.go

+
+	"knative.dev/pkg/apis/duck/v1"


This would break compilation

alvaroaleman · 2020-06-07T02:11:45Z

apis/duck/v1beta1/addressable_types.go

-	"knative.dev/pkg/apis/duck"
-	v1 "knative.dev/pkg/apis/duck/v1"
+	"knative.dev/pkg/apis/duck/ducktypes"
+	"knative.dev/pkg/apis/duck/v1"


This would break compilation

markusthoemmes · 2020-06-08T06:07:03Z

/ok-to-test

alvaroaleman · 2020-06-08T23:52:59Z

@markusthoemmes is there anything I have to do regarding the failing presubmit? I just moved code around so the test coverage shouldn't change to how it was before

alvaroaleman · 2020-06-15T15:52:05Z

@markusthoemmes @mattmoor @vagababov the issue this PR fixes is pretty problematic for downstream projects that import the knative api. The PR is apicompatible thanks to the alias so it shouldn't cause any issue.

Can you please take another look?

vagababov · 2020-06-15T16:21:12Z

/assign @vaikas @n3wscott

mattmoor

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

apis/duck/v1_tests.go

apis/duck/v1beta1/addressable_types.go

alvaroaleman · 2020-06-25T22:06:16Z

Can someone please have a look? The fact that we import these types blocks us from being able to upgrade kube dependencies. There is zero reason to import apimachinery into an api package.

knative-metrics-robot · 2020-06-25T22:08:52Z

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

File	Old Coverage	New Coverage	Delta
apis/duck/v1/addressable_types.go	100.0%	60.0%	-40.0
apis/duck/v1/podspec_types.go	100.0%	33.3%	-66.7
apis/duck/v1/status_types.go	100.0%	95.0%	-5.0
apis/duck/v1_tests.go	Do not exist	0.0%
apis/duck/v1beta1/addressable_types.go	100.0%	81.8%	-18.2
apis/duck/v1beta1/status_types.go	100.0%	82.4%	-17.6
apis/duck/v1beta1_tests.go	Do not exist	0.0%

knative-prow-robot · 2020-06-25T22:09:06Z

@alvaroaleman: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-knative-pkg-go-coverage	`c30d649`	link	`/test pull-knative-pkg-go-coverage`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

vaikas · 2020-06-25T22:15:39Z

/lgtm
/approve
/hold
Adding a hold so folks that had comments can look. I'll remove by EOD if nobody complains. @n3wscott @vagababov
My most sincere apologies, this totally fell through the cracks :(

vaikas · 2020-06-25T22:16:30Z

@alvaroaleman re: coverage, you're fine nothing else to do, not your fault :)

vagababov

/lgtm

n3wscott · 2020-06-25T22:24:09Z

I don't understand why this is needed. the stuff in duck/ defines interfaces that are used in the versions. Now ducktype is a pseudo version but the code is the same. Why is this better?

alvaroaleman · 2020-06-25T22:52:29Z

@n3wscott the problem is that knative.dev/pkg/apis/duck/v1 imports knative.dev/pkg/apis/duck which in turn imports k8s.io/apimachinery/pkg{runtime,runtime/schema,watch} (in apis/duck/typed.go).

Because there are implementations in duck/v1 for duck interfaces that themselves return an interface which is also defined in duck, the latter had to be moved out because golang apparently does not allow an interface implementation to have a return type that differs from what is in that implemented interface, even if that return type is another interface that gets implemented by the concrete return type. i.E.:

diff --git a/apis/duck/v1/addressable_types.go b/apis/duck/v1/addressable_types.go
index 3cfcac33..e53daaf0 100644
--- a/apis/duck/v1/addressable_types.go
+++ b/apis/duck/v1/addressable_types.go
@@ -24,7 +24,6 @@ import (
        "k8s.io/apimachinery/pkg/runtime"
 
        "knative.dev/pkg/apis"
-       "knative.dev/pkg/apis/duck/ducktypes"
 )
 
 // +genduck
@@ -68,7 +67,7 @@ var (
 )
 
 // GetFullType implements duck.Implementable
-func (*Addressable) GetFullType() ducktypes.Populatable {
+func (*Addressable) GetFullType() *AddressableType {
        return &AddressableType{}
 }

fails to compile:

apis/duck/v1_tests.go:48:37: cannot use &"knative.dev/pkg/apis/duck/v1".Addressable literal (type *"knative.dev/pkg/apis/duck/v1".Addressable) as type ducktypes.Implementable in field value:
	*"knative.dev/pkg/apis/duck/v1".Addressable does not implement ducktypes.Implementable (wrong type for GetFullType method)
		have GetFullType() *"knative.dev/pkg/apis/duck/v1".AddressableType
		want GetFullType() ducktypes.Populatable

Hence, those interfaces had to be moved to a new package that is imported by both api/duck and api/duck/v1

n3wscott · 2020-06-26T00:59:51Z

Sounds like you have a dep problem and the repo is not the problem. Duck/v1 also imports api machinery so it is not clear how moving the interfaces changes anything.

Are you using a very very new version of golang? Use compile this arrangement all the time in Knative and numerous other projects.

What is the exact compile issue? And are you using go mod?

alvaroaleman · 2020-06-26T01:46:44Z

@n3wscott Thanks for your feedback.

Sounds like you have a dep problem and the repo is not the problem

Yes I have a dep problem because of this repo.

Duck/v1 also imports api machinery so it is not clear how moving the interfaces changes anything.

Yes it imports some from apimachinery but much less, just k8s.io/apimachinery/pkg/apis/meta/v1 and k8s.io/apimachinery/pkg/runtime of which it just uses metav1.ObjectMeta, metav1.TypeMeta and runtime.Object, all of which are extremely stable types/interfaces.

Are you using a very very new version of golang?

1.13 but that should be irrelevant for this

Use compile this arrangement all the time in Knative and numerous other projects.

I don't understand what you mean by this, can you elaborate?

What is the exact compile issue?

https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/test-infra/17869/pull-test-infra-bazel/1269424108739760128#1:build-log.txt%3A4846
From kubernetes/test-infra#17869

That pull does not fail on knative anymore with the changes in this PR.

And are you using go mod?

Yes, but I doubt that is relevant.

alvaroaleman · 2020-06-29T19:26:40Z

@n3wscott does that explanation make sense to you? Is there anything further you would like to know? If not, I would appreciate a /hold cancel so we can proceed with updating the kube dependencies :)

alvaroaleman · 2020-07-07T21:07:40Z

@n3wscott ping

alvaroaleman · 2020-07-10T20:39:11Z

@n3wscott Can you please respond? This is still blocking us from upgrading the version of our kube dependencies even though the absolute only thing we want from this repo are the types. It is not sustainable for downstream projects if their dependencies force them to use a specific kube client-go version.

vagababov · 2020-07-10T20:41:28Z

Scott's on vacation
/cc @mattmoor @vaikas

n3wscott · 2020-07-11T00:39:17Z

/unhold

Sorry looks like you are trying to cherry pick a single class and it is at a package that pulls a ton of other stuff.

Sorry for the delay, my GitHub notifications are a... challenge 😭

Thanks for the ping!

/Lgtm
/Approve

knative-prow-robot · 2020-07-11T00:39:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, n3wscott, vaikas

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:

~~apis/duck/OWNERS~~ [n3wscott,vaikas]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 7, 2020

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 7, 2020

knative-prow-robot requested review from mattmoor and vaikas June 7, 2020 02:06

knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2020

knative-prow-robot assigned mattmoor Jun 7, 2020

mattmoor reviewed Jun 7, 2020

View reviewed changes

alvaroaleman commented Jun 7, 2020

View reviewed changes

apis/duck/v1_tests.go

Comment on lines +24 to +25

"knative.dev/pkg/apis/duck/v1"

Copy link

Contributor Author

alvaroaleman Jun 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would break compilation

alvaroaleman commented Jun 7, 2020

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 8, 2020

knative-prow-robot assigned n3wscott and vaikas Jun 15, 2020

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2020

Avoid importing apis/duck from apis/duck/<<version>>

c30d649

alvaroaleman force-pushed the try1 branch from 43efa23 to c30d649 Compare June 25, 2020 22:04

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2020

mattmoor reviewed Jun 25, 2020

View reviewed changes

apis/duck/v1_tests.go Show resolved Hide resolved

apis/duck/v1beta1/addressable_types.go Show resolved Hide resolved

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 25, 2020

vagababov reviewed Jun 25, 2020

View reviewed changes

knative-prow-robot assigned vagababov Jun 25, 2020

knative-prow-robot requested a review from mattmoor July 10, 2020 20:41

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 11, 2020

knative-prow-robot merged commit 2250202 into knative:master Jul 11, 2020


	"knative.dev/pkg/apis/duck/v1"

	v1 "knative.dev/pkg/apis/duck/v1"

Avoid importing apis/duck from apis/duck/<<version>> #1388

Avoid importing apis/duck from apis/duck/<<version>> #1388

Conversation

alvaroaleman commented Jun 7, 2020

knative-prow-robot commented Jun 7, 2020

knative-prow-robot commented Jun 7, 2020

alvaroaleman commented Jun 7, 2020

mattmoor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman commented Jun 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markusthoemmes commented Jun 8, 2020

alvaroaleman commented Jun 8, 2020

alvaroaleman commented Jun 15, 2020

vagababov commented Jun 15, 2020

mattmoor left a comment

Choose a reason for hiding this comment

alvaroaleman commented Jun 25, 2020

knative-metrics-robot commented Jun 25, 2020

knative-prow-robot commented Jun 25, 2020

vaikas commented Jun 25, 2020

vaikas commented Jun 25, 2020

vagababov left a comment

Choose a reason for hiding this comment

n3wscott commented Jun 25, 2020

alvaroaleman commented Jun 25, 2020 • edited Loading

n3wscott commented Jun 26, 2020

alvaroaleman commented Jun 26, 2020

alvaroaleman commented Jun 29, 2020

alvaroaleman commented Jul 7, 2020

alvaroaleman commented Jul 10, 2020

vagababov commented Jul 10, 2020

n3wscott commented Jul 11, 2020

knative-prow-robot commented Jul 11, 2020

alvaroaleman Jun 25, 2020 •

edited

Loading

alvaroaleman commented Jun 25, 2020 •

edited

Loading