-
Notifications
You must be signed in to change notification settings - Fork 580
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
Improve findSubnet logic #3419
Improve findSubnet logic #3419
Conversation
@pydctw: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
/test pull-cluster-api-provider-aws-e2e |
/lgtm |
e2e test failure - recently merged PR (#3425) is causing a failure. Not related to changes in this PR. Failed to run clusterctl config cluster
Unexpected error:
<*errors.withStack | 0xc000fd39e0>: {
error: <*errors.withMessage | 0xc0015d2880>{
cause: <*errors.fundamental | 0xc000fd39b0>{
msg: "a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')",
...
msg: "invalid cluster name",
},
...
}
invalid cluster name: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
occurred
/home/prow/go/pkg/mod/sigs.k8s.io/cluster-api/test@v1.1.2/framework/clusterctl/client.go:216} |
6e75a60
to
c1c0f55
Compare
New changes are detected. LGTM label has been removed. |
Will wait for #3429 to be merged. |
c1c0f55
to
3db45db
Compare
Make findSubnet to return an error when there is no matching subnet with ID
3db45db
to
b2cc995
Compare
/test pull-cluster-api-provider-aws-e2e |
subnets := s.scope.Subnets().FilterPrivate().FilterByZone(*failureDomain) | ||
if len(subnets) == 0 { | ||
errMessage := fmt.Sprintf("failed to run machine %q, no subnets available in availability zone %q", | ||
errMessage := fmt.Sprintf("failed to run machine %q with public IP, no public subnets available in availability zone %q", |
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.
Using same log for having no public subnets in the list vs having public subnets but missing their subnet IDs is misleading. Might worth keeping if len(subnets) == 0
check.
scope.Name(), *failureDomain) | ||
record.Warnf(scope.AWSMachine, "FailedCreate", errMessage) | ||
return "", awserrors.NewFailedDependency(errMessage) | ||
if subnetID := getSubnetID(subnets); subnetID != "" { |
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.
Instead of returning early when the ID is found, checking negative case first and returning early if fails is a more readable pattern to use and by this way calling getSubnetId() twice in the same case statement will not be needed.
errMessage := fmt.Sprintf("failed to run machine %q with public IP, no public subnets available", scope.Name()) | ||
record.Eventf(scope.AWSMachine, "FailedCreate", errMessage) | ||
return "", awserrors.NewFailedDependency(errMessage) | ||
if subnetID := getSubnetID(subnets); subnetID != "" { |
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.
Same comment as above.
case scope.AWSMachine.Spec.PublicIP != nil && *scope.AWSMachine.Spec.PublicIP: | ||
subnets := s.scope.Subnets().FilterPublic() | ||
if len(subnets) == 0 { |
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.
Same comment as above.
What is the status with this PR? |
Make findSubnet to return an error when there is no matching subnet with ID
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
Make findSubnet to return an error when there is no matching subnet with ID
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Part of #3399
Special notes for your reviewer:
Wrote a test case that catches the failing scenario described in the issue
Checklist: