Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move code being used by kafka plugin from client #67

Merged
merged 6 commits into from
Oct 24, 2022
Merged

Move code being used by kafka plugin from client #67

merged 6 commits into from
Oct 24, 2022

Conversation

vyasgun
Copy link
Contributor

@vyasgun vyasgun commented Oct 10, 2022

Changes

  • 🎁 Moved shared code for kafka plugin from client

/kind enhancement

Fixes partly knative/client#1591

Release Note

Docs

@knative-prow knative-prow bot added kind/enhancement 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 Oct 10, 2022
@codecov
Copy link

codecov bot commented Oct 10, 2022 •
edited
Loading

Codecov Report

Base: 78.24% // Head: 81.47% // Increases project coverage by +3.22% 🎉

Coverage data is based on head (5f413b5) compared to base (55d4794).
Patch coverage: 84.62% of modified lines in pull request are covered.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main      #67      +/-   ##
==========================================
+ Coverage   78.24%   81.47%   +3.22%     
==========================================
  Files          13       24      +11     
  Lines         832     1684     +852     
==========================================
+ Hits          651     1372     +721     
- Misses        141      239      +98     
- Partials       40       73      +33
Impacted Files Coverage Δ
pkg/kn/config/config.go 59.18% <59.18%> (ø)
pkg/dynamic/client.go 80.14% <80.14%> (ø)
pkg/printers/prefixwriter.go 83.33% <83.33%> (ø)
pkg/kn/commands/flags/sink.go 86.13% <86.13%> (ø)
pkg/dynamic/client_mock.go 90.00% <90.00%> (ø)
pkg/kn/commands/describe.go 90.90% <90.90%> (ø)
pkg/printers/tableprinter.go 92.98% <92.98%> (ø)
pkg/dynamic/lib.go 100.00% <100.00%> (ø)
pkg/printers/interface.go 100.00% <100.00%> (ø)
pkg/printers/tablegenerator.go 100.00% <100.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

dsimansk
dsimansk reviewed Oct 17, 2022
View reviewed changes
pkg/apis/client/v1alpha1/export_types.go
Comment on lines +25 to +26
// +genclient:noStatus
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need to bring in hack/update-codege.sh script as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will copy the scripts for this.

@dsimansk
Copy link
Contributor

/test all

dsimansk
dsimansk reviewed Oct 18, 2022
View reviewed changes
pkg/kn/plugin/manager.go Outdated
// Allow plugins to register to this slice for inlining
var InternalPlugins PluginList

// Interface describing a plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I think for the plugin purposes we should only extract this interface from the knative/client, maybe move it to types.go file and keep manager.go in tact in current repository. The interface is actually only common code for plugins to use.

@vyasgun vyasgun force-pushed the pr/kafka-code branch 3 times, most recently from b1e03ba to d6b2de6 Compare October 18, 2022 16:22
@vyasgun vyasgun requested a review from dsimansk October 20, 2022 08:50
dsimansk
dsimansk reviewed Oct 20, 2022
View reviewed changes
pkg/kn/plugin/types.go
// Allow plugins to register to this slice for inlining
var InternalPlugins PluginList

// Interface describing a plugin
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to keep only Plugin interface. Both PluginList []Plugin and var InternalPlugins PluginList is needed only on the knative/client side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

InternalPlugins is being used in the kafka-plugin:
https://github.com/knative-sandbox/kn-plugin-source-kafka/blob/main/plugin/plugin.go#L26

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh ok, that's the part of plugin registering to kn. Sure then we need it here.

dsimansk
dsimansk reviewed Oct 21, 2022
View reviewed changes
lib/test/cli.go Outdated
// See the License for the specific language governing permissions and
// limitations under the License.

package test
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove the lib/ top-level dir completely. Maybe move to pkg/util/test package or just pkg/util.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved lib/ to pkg/util

@dsimansk dsimansk mentioned this pull request Oct 21, 2022
Closed
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2022
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 21, 2022
@dsimansk
Copy link
Contributor

/approve
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2022
@knative-prow
Copy link

knative-prow bot commented Oct 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

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

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow bot merged commit 6cf1b33 into knative:main Oct 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsimansk dsimansk dsimansk approved these changes

Assignees

@dsimansk dsimansk

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/enhancement lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@vyasgun @dsimansk