Fix plugin inlining that uses client-pkg dependency by dsimansk · Pull Request #1816 · knative/client

dsimansk · 2023-05-24T12:14:23Z

Description

Changes

Fix plugin inlining that uses client-pkg dependency

This moves the initialization to new client-pkg, otherwise the inlined plugins depending on client-pkg aren't recognized as expected.

/cc @rhuss @vyasgun

Reference

Fixes #

Release Note

Fix plugin inlining that uses client-pkg dependency

codecov · 2023-05-24T12:22:19Z

Codecov Report

Patch coverage: 66.66% and project coverage change: -0.01 ⚠️

Comparison is base (75d62a3) 79.74% compared to head (bfaf3f9) 79.73%.

❗ Current head bfaf3f9 differs from pull request most recent head 29b7e83. Consider uploading reports for the commit 29b7e83 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1816      +/-   ##
==========================================
- Coverage   79.74%   79.73%   -0.01%     
==========================================
  Files         179      179              
  Lines       13890    13898       +8     
==========================================
+ Hits        11076    11081       +5     
- Misses       2052     2054       +2     
- Partials      762      763       +1

Impacted Files	Coverage Δ
pkg/kn/plugin/manager.go	`84.19% <66.66%> (-0.66%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

rhuss · 2023-05-24T16:37:50Z

IIUC this is about moving the pluginmanager to pkg, so that plugins can add themselves to that global plugin array (because now they don't have a dependency to the client anymore, which is what we want).

If this is correct, the chance makes totally sense. However, I would also remove moved parts from the client hier as well.

dsimansk · 2023-05-25T10:19:46Z

IIUC this is about moving the pluginmanager to pkg, so that plugins can add themselves to that global plugin array (because now they don't have a dependency to the client anymore, which is what we want).

If this is correct, the chance makes totally sense. However, I would also remove moved parts from the client hier as well.

Yep, that's exact summary. I'll clean up the moved parts in a big bang sweep. :)

dsimansk · 2023-06-19T12:19:05Z

@rhuss or @vyasgun would be able to take a look pls? I'll rebase the PR though to be mergeable again.

vyasgun · 2023-06-19T13:01:23Z

/lgtm

dsimansk · 2023-06-22T13:27:21Z

@vyasgun sorry, rebased now. :)

vyasgun · 2023-06-22T13:28:10Z

/lgtm

dsimansk · 2023-06-23T14:29:39Z

@vyasgun @rhuss if you may pls. I had to rebase due to other fix within e2e tests. Thanks!

dsimansk · 2023-06-26T09:31:46Z

/retest

vyasgun · 2023-06-29T12:17:49Z

/lgtm

dsimansk · 2023-06-30T16:01:49Z

@vyasgun I went with a different approach, rather make both lists work for a while as a backward compatibility solution.

@rhuss per our chat, that's upstream fix.

dsimansk · 2023-06-30T16:04:34Z

+	// Merge plugins that implements client-pkg Plugin type to the same manager instance
+	for _, p := range pkgplugin.InternalPlugins {
+		pluginManager.AppendPlugin(p)
+	}
+


This part is important!

There're 2 gloabal variables:

client InternalPlugins

client-pkg InternalPlugins

Depending in the plugin impl both are still valid, either by using new depedency or former on client repo. Therefore there's list merge to a single one that is used onward. I.e. content of client-pkg list is merged into client list.

dsimansk · 2023-07-03T18:18:52Z

/retest

rhuss · 2023-07-13T10:33:26Z

 	pluginManager := plugin.NewManager(config.GlobalConfig.PluginsDir(), config.GlobalConfig.LookupPluginsInPath())

+	// Merge plugins that implements client-pkg Plugin type to the same manager instance
+	for _, p := range pkgplugin.InternalPlugins {


I wonder whether we shouldn't put this internally in plugin.NewManager() where it feels more natural to me. It could be a lazy initialization (i.e. do only once regardless how often NewManager() is called). It would also keep main() a bit cleaner.

Does it look better now? Agreed on moving the scope to constructor.

@rhuss ping if you get to it.

thanks! (sorry, missed that ping)

/approve
/lgtm

dsimansk · 2023-07-14T07:52:52Z

/retest

knative-prow · 2023-07-18T07:12:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dsimansk, rhuss

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~~ [dsimansk,rhuss]

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

* Fix plugin inlining that uses client-pkg dependency * Move list merge into plugin manager * Fix imports formatting

) * Fix plugin inlining that uses client-pkg dependency * Move list merge into plugin manager * Fix imports formatting

knative-prow bot requested review from rhuss and vyasgun May 24, 2023 12:14

knative-prow bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 24, 2023

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

knative-prow bot assigned vyasgun Jun 19, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 19, 2023

dsimansk force-pushed the pr/fix-inline branch from b34d2ae to d1552ed Compare June 22, 2023 13:27

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2023

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

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2023

dsimansk force-pushed the pr/fix-inline branch from d1552ed to 2fd57d1 Compare June 23, 2023 14:29

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2023

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

dsimansk force-pushed the pr/fix-inline branch from 2fd57d1 to 4717703 Compare June 30, 2023 15:59

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 30, 2023

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

knative-prow bot 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 Jun 30, 2023

dsimansk commented Jun 30, 2023

View reviewed changes

rhuss reviewed Jul 13, 2023

View reviewed changes

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

dsimansk added 2 commits July 13, 2023 16:14

Fix plugin inlining that uses client-pkg dependency

c22cfd5

Move list merge into plugin manager

bfaf3f9

dsimansk force-pushed the pr/fix-inline branch from 72d350a to bfaf3f9 Compare July 13, 2023 14:14

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

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 13, 2023

Fix imports formatting

29b7e83

knative-prow bot assigned rhuss Jul 18, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2023

knative-prow bot merged commit e6a60c4 into knative:main Jul 18, 2023

dsimansk added this to the Knative 1.11 milestone Aug 10, 2023

dsimansk added a commit to dsimansk/client that referenced this pull request Oct 3, 2023

Fix plugin inlining that uses client-pkg dependency (knative#1816)

77b76ff

* Fix plugin inlining that uses client-pkg dependency * Move list merge into plugin manager * Fix imports formatting

Conversation

dsimansk commented May 24, 2023

Description

Changes

Reference

Uh oh!

codecov bot commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rhuss commented May 24, 2023

Uh oh!

dsimansk commented May 25, 2023

Uh oh!

dsimansk commented Jun 19, 2023

Uh oh!

vyasgun commented Jun 19, 2023

Uh oh!

dsimansk commented Jun 22, 2023

Uh oh!

vyasgun commented Jun 22, 2023

Uh oh!

dsimansk commented Jun 23, 2023

Uh oh!

dsimansk commented Jun 26, 2023

Uh oh!

vyasgun commented Jun 29, 2023

Uh oh!

dsimansk commented Jun 30, 2023

Uh oh!

dsimansk Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

dsimansk commented Jul 3, 2023

Uh oh!

rhuss Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

dsimansk Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

dsimansk Jul 13, 2023

Choose a reason for hiding this comment

Uh oh!

dsimansk Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

rhuss Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

dsimansk commented Jul 14, 2023

Uh oh!

knative-prow bot commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented May 24, 2023 •

edited

Loading