Splitting configmap package to avoid heavy dependencies #1851

mattmoor · 2020-10-27T03:56:44Z

/hold

Abusing the downstream test to see what breaks 🤞

knative-prow-robot · 2020-10-27T04:07:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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:

~~OWNERS~~ [mattmoor]

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

codecov · 2020-10-27T04:13:23Z

Codecov Report

Merging #1851 (2635282) into master (e41409a) will decrease coverage by 1.03%.
The diff coverage is 84.21%.

@@            Coverage Diff             @@
##           master    #1851      +/-   ##
==========================================
- Coverage   69.06%   68.02%   -1.04%     
==========================================
  Files         209      223      +14     
  Lines        8786     9471     +685     
==========================================
+ Hits         6068     6443     +375     
- Misses       2448     2703     +255     
- Partials      270      325      +55

Impacted Files	Coverage Δ
tracing/setup.go	`0.00% <ø> (ø)`
configmap/manual_watcher.go	`78.57% <57.14%> (-21.43%)`	⬇️
configmap/informer/informed_watcher.go	`94.20% <100.00%> (ø)`
leaderelection/context.go	`0.00% <0.00%> (-91.82%)`	⬇️
depcheck/depcheck.go	`50.00% <0.00%> (-32.15%)`	⬇️
test/logstream/v2/stream.go	`88.76% <0.00%> (-7.12%)`	⬇️
tracing/config/tracing.go	`70.31% <0.00%> (-4.69%)`	⬇️
network/h2c.go	`15.38% <0.00%> (-4.62%)`	⬇️
leaderelection/config.go	`72.72% <0.00%> (-4.55%)`	⬇️
tracing/zipkin.go	`68.42% <0.00%> (-3.01%)`	⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e41409a...59aa3ca. Read the comment docs.

dprotaso · 2020-10-27T13:31:07Z

configmap/nodep_test.go

+	nodep.AssertNoDependency(t, map[string]sets.String{
+		"knative.dev/pkg/configmap": sets.NewString(
+			"k8s.io/client-go/informers",
+		),
+	})


how does this affect test duration?

can we avoid the package name knative.dev/pkg/configmap

do we have instances where binaries are depending on the configmap water interface but don't have informers?

Test took 17s

Not AFAIK, I also specifically put this into package configmap_test to avoid polluting its dependencies (which might make this harder).

What I eyeballed yesterday showed that this wasn't pulling in the configmap.Watcher, but other things like helper functions for parsing configmap keys, and constants. I need to pull this into Serving today and see how it affects the QP dep graph.

Just curious, is your feeling that exclusion list is going to be smaller, or the allow list? Just thinking about which way we think it's easier to create these tests. If we use exclusion list, then any new dependency is allowed by default. I wonder if just like we have test coverage, we could also in our actions do things like:

show the resulting binary size changes

show the resulting binary dep deltas
etc.

Neat stuff indeed.

I renamed this package depcheck here: #1853

We can add other assertions about the dependency graph (e.g. AssertOnlyDeps), which I could see being very useful for our most widely used libraries. However, generally I think this will be prohibitive to maintain for non-leaf libraries.

mattmoor · 2020-10-27T17:47:22Z

Related: knative/serving#9957

dprotaso · 2020-10-27T18:54:39Z

Test took 17s

$ time go list -f '{{ join .Deps "\n" }}'
...
real	0m0.255s
user	0m0.760s
sys	0m0.416s

mattmoor · 2020-10-27T18:58:56Z

go test ./configmap/
ok      knative.dev/pkg/configmap       0.922s

🙃 I was going off of the test timing on Prow.

dprotaso · 2020-10-27T19:08:26Z

lol that's not 17s

dprotaso · 2020-10-27T19:10:29Z

why is prow so slow?

vagababov · 2020-10-27T19:11:00Z

Has it ever been fast?

dprotaso · 2020-10-27T19:16:31Z

also @mattmoor run go clean -testcache -cache ;)

mattmoor · 2020-10-27T19:20:55Z

I've never run go clean in my life, why start now? What problem are we trying to solve...?

The test wasn't cached, or it wouldn't print the time. 🤷

vagababov · 2020-10-27T20:35:25Z

-count=1 ensures it is not cached

mattmoor · 2020-10-28T13:43:07Z

I'm going to wait on this until after the release cut because it contains breaking changes, and we won't unlock the bigger win until we remove k8s.io/client-go/kubernetes from knative.dev/metrics

mattmoor · 2020-12-22T20:54:45Z

/test pull-knative-pkg-unit-tests

prometheus export flake 🙃

Related: knative/pkg#1851

mattmoor · 2020-12-22T21:43:35Z

I staged PRs for the three broken downstream repositories.

Tekton has a couple places to fixup, but they are just in test code.

vagababov · 2020-12-22T21:45:29Z

/lgtm

Related: knative/pkg#1851

mattmoor · 2020-12-22T21:56:09Z

/unhold

Related: knative/pkg#1851

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Oct 27, 2020

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 27, 2020

knative-prow-robot requested review from n3wscott and vagababov October 27, 2020 03:56

mattmoor force-pushed the try-splitting-configmap branch from 851bd5a to 7ded756 Compare October 27, 2020 04:07

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 27, 2020

mattmoor force-pushed the try-splitting-configmap branch from 7ded756 to 05d0484 Compare October 27, 2020 04:44

dprotaso reviewed Oct 27, 2020

View reviewed changes

mattmoor force-pushed the try-splitting-configmap branch 2 times, most recently from eab0438 to 3ad9fb1 Compare October 27, 2020 16:43

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 27, 2020

mattmoor mentioned this pull request Oct 27, 2020

queue-proxy is huge knative/serving#9957

Open

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

Try splitting configmap package

59aa3ca

mattmoor force-pushed the try-splitting-configmap branch from 3ad9fb1 to 59aa3ca Compare December 22, 2020 20:44

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

mattmoor added a commit to mattmoor/serving that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package.

ceb72d3

Related: knative/pkg#1851

mattmoor mentioned this pull request Dec 22, 2020

Prepare for splitting configmap package. knative/serving#10425

Merged

mattmoor added a commit to mattmoor/eventing that referenced this pull request Dec 22, 2020

[WIP] Prepare for splitting configmap package.

54461c7

Related: knative/pkg#1851

mattmoor mentioned this pull request Dec 22, 2020

Prepare for splitting configmap package. knative/eventing#4677

Merged

mattmoor added a commit to mattmoor/eventing-kafka that referenced this pull request Dec 22, 2020

[WIP] Prepare for splitting configmap package.

cb63c48

Related: knative/pkg#1851

mattmoor mentioned this pull request Dec 22, 2020

Prepare for splitting configmap package. knative-extensions/eventing-kafka#287

Merged

mattmoor changed the title ~~Try splitting configmap package~~ Splitting configmap package to avoid heavy dependencies Dec 22, 2020

knative-prow-robot assigned vagababov Dec 22, 2020

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020

mattmoor added a commit to mattmoor/serving that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package.

ff37fd4

Related: knative/pkg#1851

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

knative-prow-robot merged commit e2d6b4f into knative:master Dec 22, 2020

mattmoor deleted the try-splitting-configmap branch December 22, 2020 21:58

mattmoor added a commit to mattmoor/serving that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package.

dfc74bc

Related: knative/pkg#1851

mattmoor added a commit to mattmoor/eventing that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package.

2683adb

Related: knative/pkg#1851

mattmoor added a commit to mattmoor/eventing-kafka that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package.

87e6ed8

Related: knative/pkg#1851

knative-prow-robot pushed a commit to knative/eventing that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package. (#4677)

b482c5f

Related: knative/pkg#1851

knative-prow-robot pushed a commit to knative/serving that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package. (#10425)

f936091

Related: knative/pkg#1851

knative-prow-robot pushed a commit to knative-extensions/eventing-kafka that referenced this pull request Dec 22, 2020

Prepare for splitting configmap package. (#287)

3d1488f

Related: knative/pkg#1851

Harwayne mentioned this pull request Dec 23, 2020

Upgrade to latest dependencies google/knative-gcp#2020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Splitting configmap package to avoid heavy dependencies #1851

Splitting configmap package to avoid heavy dependencies #1851

mattmoor commented Oct 27, 2020

knative-prow-robot commented Oct 27, 2020

codecov bot commented Oct 27, 2020 •

edited

Loading

dprotaso Oct 27, 2020

mattmoor Oct 27, 2020

vaikas Oct 27, 2020

mattmoor Oct 27, 2020

mattmoor commented Oct 27, 2020

dprotaso commented Oct 27, 2020

mattmoor commented Oct 27, 2020

dprotaso commented Oct 27, 2020

dprotaso commented Oct 27, 2020

vagababov commented Oct 27, 2020

dprotaso commented Oct 27, 2020

mattmoor commented Oct 27, 2020

vagababov commented Oct 27, 2020

mattmoor commented Oct 28, 2020

mattmoor commented Dec 22, 2020

mattmoor commented Dec 22, 2020

vagababov commented Dec 22, 2020

mattmoor commented Dec 22, 2020

Splitting configmap package to avoid heavy dependencies #1851

Splitting configmap package to avoid heavy dependencies #1851

Conversation

mattmoor commented Oct 27, 2020

knative-prow-robot commented Oct 27, 2020

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

dprotaso Oct 27, 2020

Choose a reason for hiding this comment

mattmoor Oct 27, 2020

Choose a reason for hiding this comment

vaikas Oct 27, 2020

Choose a reason for hiding this comment

mattmoor Oct 27, 2020

Choose a reason for hiding this comment

mattmoor commented Oct 27, 2020

dprotaso commented Oct 27, 2020

mattmoor commented Oct 27, 2020

dprotaso commented Oct 27, 2020

dprotaso commented Oct 27, 2020

vagababov commented Oct 27, 2020

dprotaso commented Oct 27, 2020

mattmoor commented Oct 27, 2020

vagababov commented Oct 27, 2020

mattmoor commented Oct 28, 2020

mattmoor commented Dec 22, 2020

mattmoor commented Dec 22, 2020

vagababov commented Dec 22, 2020

mattmoor commented Dec 22, 2020

codecov bot commented Oct 27, 2020 •

edited

Loading