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.

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

🧹 Removing the vendor dir 🎆 #307

Merged
merged 1 commit into from
Sep 27, 2023

Conversation

cardil
Copy link
Contributor

@cardil cardil commented Sep 5, 2023

Changes

  • 🧹 Removing the vendor dir 🎆

/kind cleanup

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

@knative-prow
Copy link

knative-prow bot commented Sep 5, 2023

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

@knative-prow knative-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Sep 5, 2023
@cardil
Copy link
Contributor Author

cardil commented Sep 5, 2023

/test all

@knative-prow 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 Sep 5, 2023
@codecov
Copy link

codecov bot commented Sep 5, 2023

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 cardil mentioned this pull request Sep 6, 2023
17 tasks
@cardil
Copy link
Contributor Author

cardil commented Sep 6, 2023

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

@cardil cardil changed the title [WIP] 🧹 Removing the vendor dir 🎆 🧹 Removing the vendor dir 🎆 Sep 6, 2023
@cardil cardil marked this pull request as ready for review September 6, 2023 17:50
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 6, 2023
@cardil
Copy link
Contributor Author

cardil commented Sep 7, 2023

This PR is looking good.

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

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 7, 2023
@cardil cardil force-pushed the cleanup/vendorless branch 2 times, most recently from 96a6b8e to b461cd0 Compare September 25, 2023 19:52
@cardil
Copy link
Contributor Author

cardil commented Sep 26, 2023

/unhold

The Func's PR was already merged: knative/func#1966

/cc @dsimansk
/cc @rhuss

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2023
@rhuss
Copy link
Contributor

rhuss commented Sep 27, 2023

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 knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023
@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2023
@knative-prow
Copy link

knative-prow bot commented Sep 27, 2023

[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:

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

@cardil
Copy link
Contributor Author

cardil commented Sep 27, 2023

@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: knative/infra#134

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2023
@knative-prow knative-prow bot merged commit 2005194 into knative-extensions:main Sep 27, 2023
20 checks passed
@cardil cardil deleted the cleanup/vendorless branch September 27, 2023 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. 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
Development

Successfully merging this pull request may close these issues.

2 participants