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

ClusterAutoscaler: Put APIs in a separate go module #6315

Merged
merged 4 commits into from
Mar 20, 2024
Merged

ClusterAutoscaler: Put APIs in a separate go module #6315

merged 4 commits into from
Mar 20, 2024

Conversation

tenzen-y
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

I put APIs in a separate go module.

Which issue(s) this PR fixes:

Fixes #6307

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Nov 24, 2023
@k8s-ci-robot k8s-ci-robot added area/cluster-autoscaler cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2023
@tenzen-y
Copy link
Member Author

/assign @x13n

@tenzen-y
Copy link
Member Author

Also, it might be worth it to create a separate go module in https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler/provisioningrequest/client as well.

@x13n WDYT?

@alculquicondor
Copy link
Member

I don't think it's worth maintaining 3 packages. Maybe the client and APIs can be in the same package?

alculquicondor
alculquicondor reviewed Nov 24, 2023
View reviewed changes
cluster-autoscaler/provisioningrequest/apis/go.mod Outdated
@@ -0,0 +1,47 @@
module sigs.k8s.io/autoscaler/cluster-autoscaler/provisioningrequest/apis
Copy link
Member

Choose a reason for hiding this comment

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

I think the module should be
sigs.k8s.io/autoscaler/cluster-autoscaler/apis

where one of the folders is provisioningrequest.

Copy link
Member

Choose a reason for hiding this comment

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

The reason being that we might have an generic (independent of the cloud provider) implementation of provisioningrequest and it might have dependencies that are not worth leaking to users of the API.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see. Maybe, we can put a client folder on sigs.k8s.io/autoscaler/cluster-autoscaler/apis/provisioningrequest/client as well.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't sigs.k8s.io resolve to kubernetes-sigs org? This repo is in main kubernetes org, so shouldn't it be under k8s.io/autoscaler/cluster-autoscaler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Sorry, you're right.

@tenzen-y
Copy link
Member Author

I don't think it's worth maintaining 3 packages. Maybe the client and APIs can be in the same package?

It makes sense.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 3, 2023
@tenzen-y tenzen-y force-pushed the put-apis-separate-mod branch 3 times, most recently from 9586a26 to 3dc86e8 Compare December 3, 2023 10:25
@tenzen-y tenzen-y changed the title ClusterAutoscaler: Put APIs in a separate go module WIP: ClusterAutoscaler: Put APIs in a separate go module Dec 3, 2023
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 3, 2023
@tenzen-y tenzen-y force-pushed the put-apis-separate-mod branch 3 times, most recently from ad38998 to f4aa3ef Compare December 3, 2023 13:25
@tenzen-y tenzen-y force-pushed the put-apis-separate-mod branch 3 times, most recently from 129715a to 759709f Compare December 19, 2023 21:38
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 19, 2023
@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 2, 2024

I have rebased this PR to resolve conflicts.

@tenzen-y
Copy link
Member Author

tenzen-y commented Mar 4, 2024

Hi @mwielgus! Could you take a look this PR since we need to get approve from you due to changing of hack/for-go-proj.sh.
Thanks.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2024
@village-way village-way mentioned this pull request Mar 9, 2024
Closed
@tenzen-y
ClusterAutoscaler: Put APIs in a separate go module
bd32754 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@tenzen-y
Extend update-deps.sh so that we can automatically update k8s librari…
108385e 
…es in the apis pkg

Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 9, 2024
@tenzen-y
Clean up update-deps.sh
16b213c 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 9, 2024
tenzen-y
tenzen-y commented Mar 9, 2024
View reviewed changes
cluster-autoscaler/hack/update-deps.sh
Copy link
Member Author

Choose a reason for hiding this comment

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

@x13n As you proposed in another thread, I cleaned up this script.
We can verify this script with ./hack/update-deps.sh ${K/K_ALPHA_VERSION} ${STABLE_APIS_VERSION} like ./hack/update-deps.sh v1.30.0-alpha.3 v1.29.2.

@tenzen-y
Update apis version to v1.29.2
798d611 
Signed-off-by: Yuki Iwai <yuki.iwai.tz@gmail.com>
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 9, 2024
@x13n
Copy link
Member

x13n commented Mar 15, 2024

Thanks! LGTM from my perspective.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 15, 2024
@x13n
Copy link
Member

x13n commented Mar 15, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 15, 2024
@tenzen-y
Copy link
Member Author

@mwielgus @MaciekPytel @gjtempleton Could you get a chance to review this PR?

mwielgus
mwielgus approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@mwielgus mwielgus left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mwielgus, tenzen-y, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2024
@k8s-ci-robot k8s-ci-robot merged commit 09954b6 into kubernetes:master Mar 20, 2024
6 checks passed
@tenzen-y tenzen-y deleted the put-apis-separate-mod branch March 20, 2024 16:40
This was referenced Mar 20, 2024
Merged
Merged
@azylinski azylinski mentioned this pull request Mar 28, 2024
Closed
@pmendelski pmendelski mentioned this pull request Apr 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@x13n x13n x13n left review comments

@alculquicondor alculquicondor alculquicondor left review comments

@mwielgus mwielgus mwielgus approved these changes

@BigDarkClown BigDarkClown Awaiting requested review from BigDarkClown

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@x13n x13n

@mwielgus mwielgus

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cluster-autoscaler 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. lgtm "Looks good to me", 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.

CA: Put APIs in a separate go module
6 participants
@tenzen-y @alculquicondor @x13n @elmiko @k8s-ci-robot @mwielgus