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

cleanup: remove vendoring #834

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

cleanup: remove vendoring #834

wants to merge 3 commits into from

Conversation

binacs
Copy link
Member

@binacs binacs commented Nov 24, 2024

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #760

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 24, 2024
@netlify Netlify
Copy link

netlify bot commented Nov 24, 2024 •
edited
Loading

✅ Deploy Preview for kubernetes-sigs-scheduler-plugins canceled.

Name Link
🔨 Latest commit 666529d
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-scheduler-plugins/deploys/6745d6fd6a54c400080064fc

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: binacs
Once this PR has been reviewed and has the lgtm label, please assign denkensk for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 24, 2024
@binacs binacs mentioned this pull request Nov 24, 2024
Open
4 tasks
@binacs
Copy link
Member Author

binacs commented Nov 24, 2024

cc @Huang-Wei

PTAL, thanks!

Huang-Wei
Huang-Wei reviewed Nov 24, 2024
View reviewed changes
Copy link
Contributor

@Huang-Wei Huang-Wei left a comment

Choose a reason for hiding this comment

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

Thank you so much, @binacs!

Could you please help with the following?

  1. Split the PR into two commits:
  • One commit for the code changes only
  • Another commit for the removal of the vendor/ directory
  1. Update hack/update-codegen.sh:
  • Ensure the script runs successfully and generates no differences in the current codebase
  • Test it by making some mock changes (e.g., removing some API fields) and verify that it produces the expected output

@binacs
Copy link
Member Author

binacs commented Nov 24, 2024 •
edited
Loading

hi @Huang-Wei , thanks for review!

I have split all changes into two commits as required. Modified the hack/verify-crdgen.sh and ensure that the codebase does not change. I have done relevant tests locally and it seems to be ok.

@Huang-Wei
Copy link
Contributor

Modified the hack/verify-crdgen.sh and ensure that the codebase does not change.

I meant hack/update-codegen.sh, not hack/verify-crdgen.sh.

@binacs
Copy link
Member Author

binacs commented Nov 26, 2024

hi @Huang-Wei , I pushed a commit to modify hack/update-codegen.sh and it seems to work fine (no diffs are generated).

@binacs ➜ /workspaces/scheduler-plugins (binacs/remove-vendoring) $ ./hack/update-codegen.sh 
Generating deepcopy code for 2 targets
Generating defaulter code for 1 targets
Generating conversion code for 1 targets
Generating applyconfig code for 1 targets
Generating client code for 1 targets
Generating lister code for 1 targets
Generating informer code for 1 targets

BTW, I modified the hack/verify-crdgen.sh before to make sure that we will only generate NodeResourceTopology#v1alpha2 apis (according to https://github.com/kubernetes-sigs/scheduler-plugins/blob/master/manifests/noderesourcetopology/crd.yaml).

master's vendor[Only v1alpha2]: https://github.com/kubernetes-sigs/scheduler-plugins/tree/master/vendor/github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology

upstream repo [Both v1alpha1 and v1alpha2]: https://github.com/k8stopologyawareschedwg/noderesourcetopology-api/tree/master/pkg/apis/topology)

# Only represent the github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis
- api_paths="XXX;./vendor/github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/...;XXX"
+ api_paths="XXX;github.com/k8stopologyawareschedwg/noderesourcetopology-api/pkg/apis/topology/v1alpha2;XXX"

Please take a look, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Huang-Wei Huang-Wei Huang-Wei left review comments

@PiotrProkop PiotrProkop Awaiting requested review from PiotrProkop

@zwpaper zwpaper Awaiting requested review from zwpaper

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. 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.

request: remove vendoring from this repo
3 participants
@binacs @k8s-ci-robot @Huang-Wei