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

Add network CRDs to cloud-provider-gcp #375

Merged
merged 3 commits into from Oct 19, 2022
Merged

Add network CRDs to cloud-provider-gcp #375

merged 3 commits into from Oct 19, 2022

Conversation

sdmodi
Copy link

@sdmodi sdmodi commented Oct 11, 2022

These CRDs will be used by the nodeipam controller for clusters that support multiple networks

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 11, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @sdmodi. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 11, 2022
@MrHohn
Copy link
Member

MrHohn commented Oct 11, 2022

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 11, 2022
Add network CRDs to cloud-provider-gcp
859bbce 
These CRDs will be used by the nodeipam controller for clusters
that support multiple networks
@sdmodi
Copy link
Author

sdmodi commented Oct 12, 2022

/test cloud-provider-gcp-verify-all

@sdmodi
Copy link
Author

sdmodi commented Oct 17, 2022

/test cloud-provider-gcp-verify-all

MrHohn
MrHohn approved these changes Oct 18, 2022
View reviewed changes
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

The changes look reasonable as to introduce a new set of CRDs to the /crd folder and should have no impact on the existing components. Will leave it to the actual owners of the CRDs and generation script for LGTM.
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, sdmodi

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 Oct 18, 2022
@sugangli
Copy link
Contributor

/lgtm for the update-codegen.sh

nathanperkins
nathanperkins suggested changes Oct 19, 2022
View reviewed changes
Copy link

@nathanperkins nathanperkins left a comment

Choose a reason for hiding this comment

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

I'm assuming everything in apis/network is already in use and cannot be changed at this point, so no use in reviewing.

crd/hack/update-codegen.sh Outdated
echo "Generating deepcopy functions and CRD artifacts"
go run sigs.k8s.io/controller-tools/cmd/controller-gen \
object:headerFile=${SCRIPT_ROOT}/hack/boilerplate.go.txt \
crd:crdVersions=v1 \
Copy link

Choose a reason for hiding this comment

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

Suggested change
crd:crdVersions=v1 \
crd:crdVersions=$version \

Copy link
Author

Choose a reason for hiding this comment

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

Done

crd/hack/update-codegen.sh Outdated
crd:crdVersions=v1 \
paths=${apis_pkg}/... \
output:crd:artifacts:config=${SCRIPT_ROOT}/config/crds
generate_config crd_name version
Copy link

Choose a reason for hiding this comment

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

The crd_name and version vars also don't seem to have values defined.

Suggested change
generate_config crd_name version
generate_config $crd_name $version

Copy link
Author

Choose a reason for hiding this comment

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

Done

@bowei
Copy link
Member

bowei commented Oct 19, 2022

/assign

@MrHohn
Copy link
Member

MrHohn commented Oct 19, 2022

Looks like we have gotten review froms the authors.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 19, 2022
@k8s-ci-robot k8s-ci-robot merged commit 43757f3 into kubernetes:master Oct 19, 2022
@jprzychodzen jprzychodzen mentioned this pull request Oct 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nathanperkins nathanperkins nathanperkins requested changes

@MrHohn MrHohn MrHohn approved these changes

@jpbetz jpbetz Awaiting requested review from jpbetz

@jprzychodzen jprzychodzen Awaiting requested review from jprzychodzen

Assignees

@bowei bowei

@MrHohn MrHohn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

None yet
7 participants
@sdmodi @k8s-ci-robot @MrHohn @sugangli @bowei @liggitt @nathanperkins