knative-extensions / kn-plugin-event

Kn plugin for sending events to Knative sinks.
Apache License 2.0
7 stars 22 forks source link

:broom: Removing the vendor dir :fireworks: #307

Closed cardil closed 11 months ago

cardil commented 1 year ago

Changes

/kind cleanup

Tests knative/hack#311 Tests knative/hack#222

knative-prow[bot] commented 1 year ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

cardil commented 1 year ago

/test all

codecov[bot] commented 1 year ago

Codecov Report

Merging #307 (13a1178) into main (d9a132a) will not change coverage. Report is 3 commits behind head on main. The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #307   +/-   ##
=======================================
  Coverage   68.17%   68.17%           
=======================================
  Files          35       35           
  Lines        1128     1128           
=======================================
  Hits          769      769           
  Misses        299      299           
  Partials       60       60           
cardil commented 1 year ago

The failure https://github.com/knative-extensions/kn-plugin-event/actions/runs/6089754374/job/16523241031?pr=307 is expected, as in order to use https://github.com/knative/hack/pull/222 and https://github.com/knative/hack/pull/311 I needed to use the replace stanza in go.mod which is reported by the linter.

cardil commented 1 year ago

This PR is looking good.

However, I'll put this on hold, since I'll be on PTO next week.

/hold

cardil commented 11 months ago

/unhold

The Func's PR was already merged: https://github.com/knative/func/pull/1966

/cc @dsimansk /cc @rhuss

rhuss commented 11 months ago

Well, I think this is fine. I can't dive into the PR itself, and not sure what other side-effects will happen when we inline the plugin into kn, which still uses the vendor dir. If you are sure that this doesn't harm (i.e. still works), happy to merge this (so feel free to unhold then)

/approve /lgtm /hold

knative-prow[bot] commented 11 months ago

[APPROVALNOTIFIER] This PR is APPROVED

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

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](https://github.com/knative-extensions/kn-plugin-event/blob/main/OWNERS)~~ [cardil,rhuss] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
cardil commented 11 months ago

@rhuss Well, I think this is fine. I can't dive into the PR itself, and not sure what other side-effects will happen when we inline the plugin into kn, which still uses the vendor dir. If you are sure that this doesn't harm (i.e. still works), happy to merge this (so feel free to unhold then)

Vendorless applies only to upstream projects. The vendors like RH will probably continue to vendor deps on their products, just to have a way to apply patches if needed.

Also, vendorless project doesn't change anything when the plugin is being embedded into other, like in kn.

See the epic for more info: https://github.com/knative/infra/issues/134

/unhold