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

Fix findSubnet function's logic when subnet ID is specified #2728

Merged
merged 1 commit into from
Sep 8, 2021
Merged

Fix findSubnet function's logic when subnet ID is specified #2728

merged 1 commit into from
Sep 8, 2021

Conversation

pydctw
Copy link
Contributor

@pydctw pydctw commented Sep 1, 2021 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
findSubnet function's logic is incorrect when subnet ID is specified in AWSMachine's spec. The logic should find a subnet regardless of a failureDomain setting. Current logic returns a subnet ID from a spec when failureDomain is not defined, which may not match any subnets provided in AWSCluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
#2732

Special notes for your reviewer:

Checklist:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

Release note:

Fix findSubnet function's logic when subnet ID is specified. It will find a matching subnet regardless of a failureDomain setting.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority labels Sep 1, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @pydctw. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 1, 2021
@sedefsavas
Copy link
Contributor

/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 Sep 2, 2021
@sedefsavas
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 2, 2021
@sedefsavas
Copy link
Contributor

Nit. We usually create an issue before the PR especially if the issue is a bug, then open the PR referring that issue.

@pydctw
Copy link
Contributor Author

pydctw commented Sep 2, 2021

Nit. We usually create an issue before the PR especially if the issue is a bug, then open the PR referring that issue.

Thank you for the instruction. Didn't know that. Created an issue and linked to the PR description.

@shivi28
Copy link
Contributor

shivi28 commented Sep 5, 2021

/lgtm

Fix findSubnet function's logic when subnet ID is specified
64dae2f 
When subnet ID is specified in AWSMachine's spec, it should find the
subnet whether a failureDomain is specified or not.
@pydctw pydctw force-pushed the fix-findSubnet-logic branch from f920b86 to 64dae2f Compare September 7, 2021 14:52
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2021
@pydctw
Copy link
Contributor Author

pydctw commented Sep 7, 2021

@sedefsavas, could you approve the PR?

@sedefsavas
Copy link
Contributor

/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 Sep 8, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sedefsavas

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 Sep 8, 2021
@k8s-ci-robot k8s-ci-robot merged commit e948076 into kubernetes-sigs:main Sep 8, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.7.0 milestone Sep 8, 2021
@pydctw pydctw mentioned this pull request Sep 15, 2021
Closed
7 tasks
@pydctw pydctw deleted the fix-findSubnet-logic branch October 5, 2021 16:36
enxebre added a commit to enxebre/hypershift that referenced this pull request Nov 23, 2021
@enxebre
Changes to update capa v1beta1 and support golang 1.17
0c77b1a 
Bring CAPA v1beta1, kube 1.23 deps and golang 1.17.
Drops https://github.com/bombsimon/logrusr/ which does not support golang 1.17 in favour of zapr.
Adds .ci-operator.yaml so upcoming golang bumps can be done in this repo atomically.

Note the TODO in HostedCluster
This kubernetes-sigs/cluster-api-provider-aws#2728 broke our assumption in CAPA 0.7 for externally managed infrastructure. This effectively limit our ability to span NodePools across multiple subnets. In a follow up we need to either enable upstream back to support arbitrary subnets IDs in the awsMachine CR or possibly expose a slice of available subnets for NodePools in hcluster.Spec.Platform.AWS.
@enxebre enxebre mentioned this pull request Nov 23, 2021
Merged
@richardcase richardcase mentioned this pull request Nov 29, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shivi28 shivi28 shivi28 left review comments

@randomvariable randomvariable Awaiting requested review from randomvariable

Assignees

@shivi28 shivi28

@sedefsavas sedefsavas

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. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging this pull request may close these issues.
4 participants
@pydctw @k8s-ci-robot @sedefsavas @shivi28