-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Drop autoscaler vendor directory in favor of Go modules #6572
Drop autoscaler vendor directory in favor of Go modules #6572
Conversation
d31d575
to
4d43640
Compare
@fmuyassarov feel free to mention me here once we are past the 2 months mark. I will take a look! |
As agreed originally, the plan to land this PR was in two months. This means, we should expect the PR to be merged close to the end of April, 2024 if there are no objections. |
Signed-off-by: Feruzjon Muyassarov <[email protected]>
4d43640
to
8ee8331
Compare
56babdc
to
0077481
Compare
/hold cancel |
@x13n @Shubham82 @vadasambar PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for taking the time to broadcast this for the community.
lgtm from me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure make build-in-docker
works after these changes? Current version seems unhappy about the vendor being missing.
/assign |
0077481
to
336046b
Compare
Can you please share what issue are you facing when building the binary? Because, I just tested it and didn't catch an issue.
|
Hm... interesting. For me it doesn't work, perhaps some difference in docker vs docker cli experimental?
|
FWIW, I pruned all cached containers and images and the result is still the same. |
this is odd, but thanks for the try. I will try to reproduce it on my end. |
Hi @x13n , I could try in some other machine/vm/container but I expect that the result will be the same. |
Must be something on my end. Both codespace and cloning a new repo on my machine worked correctly. No reason to block this PR then. Thanks for the contribution - I'm a fan of improvements with negative code delta and the one here is huge :) /approve I just noticed the warning in Makefile is going to become obsolete, can you drop it? |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fmuyassarov, x13n 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 |
…ling Signed-off-by: Feruzjon Muyassarov <[email protected]>
336046b
to
f318381
Compare
Good catch. Fixed. |
Thanks! /lgtm |
What type of PR is this?
/kind cleanup
/kind deprecation
What this PR does / why we need it:
Drop auto-scaler vendor directory in favor of Go modules. We're starting the process of moving away
from our auto-scaler vendor directory to adopt Go modules. This pull request will be paused for at least
two months (as agreed in #4878) to ensure the community has enough time to update their dependent
systems.
/hold
Which issue(s) this PR fixes:
Fixes # #4878
Special notes for your reviewer:
An echo statement for the build Makefile target in #6573.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: