issuer/route53: fix delete for 'NotExist' errors

[euank]

Fixes #736.

Prior to this change, it was quite possible to end up with a queue of
cleanup tasks that would never succeed.

Release note:

Fixed Route53 cleanup errors for already deleted records.

[euank]

Running ./hack/update-deps.sh isn't providing any diff for me here despite the verify fail.

I'm using:

$ dep version
dep:
 version     : v0.4.1-264-g45be32ba
 build date  : 2018-07-17
 git hash    : 45be32ba
 go version  : go1.10.1
 go compiler : gc
 platform    : linux/amd64
 features    : ImportDuringSolve=false

I ensured I was on a completely clean checkout of this PR.

I assume this is due to a difference in dep version.

[munnerz]

Also make sure you rebase onto master too. Our CI tests against the result of merging your changeset with master, so if anything has changed you'll see this kind of behaviour. FWIW, this is where the image for that test is defined: https://github.com/jetstack/test-infra/blob/master/images/gcloud/Dockerfile#L39

…

On Thu, 19 Jul 2018 at 19:21, Euan Kemp ***@***.***> wrote: Running ./hack/update-deps.sh isn't providing any diff for me here despite the verify fail. I'm using: $ dep version dep: version : v0.4.1-264-g45be32ba build date : 2018-07-17 git hash : 45be32ba go version : go1.10.1 go compiler : gc platform : linux/amd64 features : ImportDuringSolve=false I ensured I was on a completely clean checkout of this PR. I assume this is due to a difference in dep version. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAMbPyL7ezYjyhP-LRZa0H4mXXmWAQeqks5uIM4_gaJpZM4VWyyU> .

[euank]

Reverting dep back to 0.4.1 seems to find the verify issue this was hitting. I find it quite an odd issue though; I'm surprised the input digest changed even though I didn't change "Gopkg.toml" at all.

edit: turns out imports are also an input to the digest function, so e.g. the first reference to the "awserr" package in our code, even though it was already in the vendor folder, changed the digest. My dep didn't catch it because they've since removed the input-digest.

[munnerz]

Looking at the AWS docs, ErrCodeInvalidChangeBatch sounds more like a wrapper type for multiple errors from the AWS API? I think this is still okay given the limited set of operations we perform/errors that could occur, but might it be worth logging the returned error somewhere just in case we get some other weird error responses here and someone needs to open an issue/debug?

[euank]

I don't think InvalidChangeBatch will be possible for anything else when the transaction is just a single delete (as is the case here), but no harm logging it.

[munnerz]

I guess the likes of 503 errors etc are handled with different error response types.

Sounds good to me. I'll lgtm and approve after you've added the log line, if you think it is useful 🙂

[munnerz]

/approve

[munnerz]

/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz

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:

• OWNERS [munnerz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment