openshift / aws-account-operator

Operator to manage pool of AWS accounts for Hive

Apache License 2.0

32 stars 75 forks source link

[OSD-9246] Block gaps in our AccountClaim finalizer handling #675

Closed dkeohane closed 2 years ago

dkeohane commented 2 years ago

While investigating dangling accounts I looked through our accountclaim finalize functionality to try identify areas where we may not be returning errors

Previous behaviours which have been addressed as part of this PR:

If the accountclaim finalizeAccountClaim function encountered an error, we would still proceed to remove the finalizer.
If the cleanUpAwsAccount func encountered an error, we simply logged it instead of returning it.

Additions:

Added some more unit tests.
Made errors from AWS visible in our logging, as debugging why we can't clean up an AWS resource without the context from AWS was impossible.

Ticket ref: OSD-9246

codecov-commenter commented 2 years ago

Codecov Report

Merging #675 (647df84) into master (b986217) will increase coverage by 0.85%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #675      +/-   ##
==========================================
+ Coverage   31.46%   32.31%   +0.85%     
==========================================
  Files          48       48              
  Lines        4783     4787       +4     
==========================================
+ Hits         1505     1547      +42     
+ Misses       3180     3133      -47     
- Partials       98      107       +9

Impacted Files	Coverage Δ
...controller/accountclaim/accountclaim_controller.go	`22.60% <33.33%> (+1.09%)`	:arrow_up:
pkg/controller/accountclaim/reuse.go	`53.79% <33.33%> (+12.54%)`	:arrow_up:
pkg/localmetrics/localmetrics.go	`52.77% <0.00%> (+0.79%)`	:arrow_up:

iamkirkbater commented 2 years ago

Can we squash this before we merge, please.

openshift-ci[bot] commented 2 years ago

@dkeohane: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).

iamkirkbater commented 2 years ago

/hold cancel

iamkirkbater commented 2 years ago

/lgtm

openshift-ci[bot] commented 2 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkeohane, iamkirkbater, NautiluX

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](https://github.com/openshift/aws-account-operator/blob/master/OWNERS)~~ [NautiluX,iamkirkbater] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment