Aws dns cleanup refactor #1107
Aws dns cleanup refactor #1107
Conversation
openshift-ci-robot
added
the
approved
twiest
left a comment
twiest
left a comment
There was a problem hiding this comment.
Looks good, 1 minor nit.
I don't feel strongly about it, so I'll lgtm if you want to keep it as is. :)
| return err | ||
| } | ||
|
|
||
| func (a *AWSActuator) deleteRecordSets(logger log.FieldLogger) error { |
There was a problem hiding this comment.
Is this function still needed?
Would it be better to just have everything call DeleteAWSRecordSets directly?
|
|
||
| // zoneID is the ID of the hosted zone in route53 | ||
| zoneID *string | ||
| // hostedZone is the AWS object representing the hosted zone in route53 |
There was a problem hiding this comment.
While I do not object to this change, it is not clear to me why it is needed.
There was a problem hiding this comment.
Just when comparing to the other actuators, it seemed odd that we didn't have access to the full object after we went through the trouble of fetching it. But you're right, it has no immediate effect other that making the aws actuator more similar to the other actuators.
pkg/installmanager/dnscleanup.go
Outdated
| if dnsZone.Status.AWS == nil { | ||
| return fmt.Errorf("found non-AWS DNSZone for AWS ClusterDeployment") | ||
| } | ||
| if dnsZone.Status.AWS.ZoneID == nil { | ||
| // Shouldn't really be possible as we block install until DNS is ready: | ||
| return fmt.Errorf("DNSZone %s has no ZoneID set", dnsZone.Name) | ||
| } |
There was a problem hiding this comment.
I think it would be cleaner if this code were in cleanupAWSDNSZone instead.
| if err := a.ModifyStatus(); err != nil { | ||
| logger.WithError(err).Error("failed to populate DNSZone status") | ||
| return err | ||
| } |
There was a problem hiding this comment.
This is snuck in the "just store entire aws dnszone" commit. Should it be in the "populate DNSZone status immediately after fetching zoneID (AWS)" commit instead?
There was a problem hiding this comment.
Yes, it came up while testing things and i must have squashed the wrong commits together while cleaning up the commits for submission.
joelddiaz
force-pushed
the
aws-dns-cleanup-second
branch
from
85bbea7 to
8d509a9
Compare
|
Looks good to me as well. One of us can lgtm after the squash. |
Rather than two similar DNS cleanup functions, unify onto one implementation that can be used by all callers. This means we now remove more than just A records on a failed intall. Also, populate DNSZone status immediately after fetching zoneID during Refresh() and Create() Add test case to cover DNSZone being deleted without status. Align with other actuators and just store entire AWS dnszone after receiving it.
joelddiaz
force-pushed
the
aws-dns-cleanup-second
branch
from
8d509a9 to
de7dd1d
Compare
|
squashed and updated git commit text (and PR text as well) |
|
/lgtm |
|
/retest |
1 similar comment
|
/retest |
|
@twiest i think the bot took an unplanned vacation and missed your lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelddiaz, twiest 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 |
5 participants
Try number 2 to use one AWS DNS cleanup implementation.
Rework the DNSZone controller's AWS actuator code for DNS zone cleanup so that it is an exported function that can also be called during a failed cluster installation.
Rather than two similar DNS cleanup functions, unify onto one implementation that can be used by all callers.
This means we now remove more than just A records on a failed intall.
Also, populate DNSZone status immediately after fetching zoneID during Refresh() and Create()
Add test case to cover DNSZone being deleted without status.
Align with other actuators and just store entire AWS dnszone after receiving it.