Bug 1866299: Refactors AWS Provider Validation #457
Conversation
openshift-ci-robot
added
the
bugzilla/severity-high
|
@danehans: This pull request references Bugzilla bug 1866299, which is invalid:
Comment DetailsIn response to this:
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. |
openshift-ci-robot
added
bugzilla/invalid-bug
|
/bugzilla refresh |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
@danehans: This pull request references Bugzilla bug 1866299, which is valid. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
DetailsIn response to this:
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. |
openshift-ci-robot
removed
the
bugzilla/invalid-bug
|
/test e2e-aws |
Miciah
left a comment
Miciah
left a comment
There was a problem hiding this comment.
Just a couple minor suggestions. This looks like a good approach.
pkg/dns/aws/dns.go
Outdated
| // validateServiceEndpoints validates that provider clients can communicate | ||
| // with associated API endpoints by each making a list/describe/get call. | ||
| func validateServiceEndpoints(provider *Provider) error { | ||
| if provider.route53 != nil { |
There was a problem hiding this comment.
Can provider.route53, provider.elb, provider.elbv2, or provider.tags ever be nil?
There was a problem hiding this comment.
Not the way it's being called from NewProvider() today. I'll update to remove the nil check.
pkg/dns/aws/dns.go
Outdated
| return fmt.Errorf("failed to get group tagging resources: %v", err) | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
What do you think of validating all endpoints, accumulating errors, and returning the result at the end? Something like this:
var errs []error
if _, err := ...; err != nil {
errs = append(errs, fmt.Errorf(...))
}
if _, err := ...; err != nil {
errs = append(errs, fmt.Errorf(...))
}
if _, err := ...; err != nil {
errs = append(errs, fmt.Errorf(...))
}
if _, err := ...; err != nil {
errs = append(errs, fmt.Errorf(...))
}
return kerrors.NewAggregate(errs)(If errs is empty, kerrors.NewAggregate(errs) returns nil.)
There was a problem hiding this comment.
Ah, yes I like that approach. Let me update.
danehans
force-pushed
the
bz1866299_update_validation
branch
from
264e632 to
ddc335f
Compare
|
@Miciah commit |
/test e2e-aws |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danehans, Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@danehans: All pull requests linked via external trackers have merged:
Bugzilla bug 1866299 has been moved to the MODIFIED state. DetailsIn response to this:
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. |
5 participants
Previously, an AWS provider would be used without validating that clients could communicate with associated API endpoints. This PR adds validation by ensuring each AWS provider client can make a list/describe API call.
/assign @Miciah @knobunc
/cc @frobware @sgreene570