Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix unnecessary lastUnpacked status updates #397

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

m1kola
Copy link
Member

@m1kola m1kola commented Sep 12, 2024

We should not be updating lastUnpacked status fields when we read from the cache

Fixes #389

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2024
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.55%. Comparing base (8e71e93) to head (9e007c1).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #397      +/-   ##
==========================================
+ Coverage   35.29%   35.55%   +0.26%     
==========================================
  Files          17       17              
  Lines         731      734       +3     
==========================================
+ Hits          258      261       +3     
  Misses        443      443              
  Partials       30       30              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

perdasilva
perdasilva previously approved these changes Sep 12, 2024
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ^^

@perdasilva
Copy link
Contributor

doh - didn't realize it was draft - apologies

@m1kola m1kola marked this pull request as ready for review September 12, 2024 16:00
@m1kola m1kola requested a review from a team as a code owner September 12, 2024 16:00
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 12, 2024
@m1kola
Copy link
Member Author

m1kola commented Sep 12, 2024

Ready for review now

perdasilva
perdasilva previously approved these changes Sep 13, 2024
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously, we'd be updating the last unpacked time on every reconcile if the unpack directory (given by the catalog name and the image digest) already exists. In this PR we address this by checking if the ResolvedSource.Image is set and taking the last unpack time from that. Otherwise, set last unpacked to now. LGTM.

My only question is whether we ought to add a test where Status.ResolvedSource or Status.ResolvedSource.Image isn't set.

@m1kola m1kola marked this pull request as draft September 13, 2024 07:26
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2024
We should not be updating lastUnpacked status fields when
we read from the cache

Signed-off-by: Mikalai Radchuk <[email protected]>
@m1kola
Copy link
Member Author

m1kola commented Sep 13, 2024

My only question is whether we ought to add a test where Status.ResolvedSource or Status.ResolvedSource.Image isn't set.

@perdasilva this question made me think that we should probably set LastUnpacked to "now" if we do not have a value in the status.

We already had several test cases where we do not set this status field, but we had not assertions on ResolvedSource.Image.LastUnpacked in the result struct. Now we do: we check that the LastUnpacked is not zero time value.

PR updated. Please take another look.

@m1kola m1kola marked this pull request as ready for review September 13, 2024 08:28
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2024
Copy link
Contributor

@perdasilva perdasilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ^^ thank you!

@m1kola m1kola added this pull request to the merge queue Sep 13, 2024
Merged via the queue into operator-framework:main with commit dbdce9b Sep 13, 2024
13 checks passed
@m1kola m1kola deleted the fix_lastUnpacked branch September 13, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

status.lastUnpacked is being updated on every pull attempt
2 participants