openshift / aws-account-operator

Operator to manage pool of AWS accounts for Hive

Apache License 2.0

32 stars 75 forks source link

Update timestamp #634

Closed katherinelc321 closed 3 years ago

katherinelc321 commented 3 years ago

https://issues.redhat.com/browse/OSD-7694

openshift-ci[bot] commented 3 years ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: katherinelc321 To complete the pull request process, please assign jharrington22 after the PR has been reviewed. You can assign the PR to them by writing /assign @jharrington22 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files: - **[OWNERS](https://github.com/openshift/aws-account-operator/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

katherinelc321 commented 3 years ago

The SetAccountCondition func only returns conditions when we first set them but not when we update them. SetAccountStatus in status.go expects something returned in the other side of awsAccount.Status.Conditions

codecov-commenter commented 3 years ago

Codecov Report

Merging #634 (1a6ba61) into master (82a0c9f) will increase coverage by 0.00%. The diff coverage is 0.00%.

@@           Coverage Diff           @@
##           master     #634   +/-   ##
=======================================
  Coverage   31.30%   31.31%           
=======================================
  Files          46       46           
  Lines        4638     4637    -1     
=======================================
  Hits         1452     1452           
+ Misses       3086     3085    -1     
  Partials      100      100

Impacted Files	Coverage Δ
pkg/controller/utils/conditions.go	`27.67% <0.00%> (+0.17%)`	:arrow_up:

dkeohane commented 3 years ago

Were you able to confirm this fixes the issue through testing locally?

katherinelc321 commented 3 years ago

Were you able to confirm this fixes the issue through testing locally?

Not through CRC and unit tests weren't working because of the way this line was written but that's getting refactored in PR 603 and that should make my test work

dkeohane commented 3 years ago

I would not wait on 603 to be merged for making your changes - it is sitting there with quite a while, and still requires a bit of discussion around it. I would prioritise testing this with your local CRC to ensure to acts as expected