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

feat(STONEINTG-781): remove finalizer sooner for integration plr #626

Merged
merged 1 commit into from
Apr 17, 2024

Conversation

Josh-Everett
Copy link
Contributor

@Josh-Everett Josh-Everett commented Mar 14, 2024

Maintainers will complete the following section

@Josh-Everett Josh-Everett changed the title WIP: feat(STONEINTG-781): remove finalizer sooner for integration plr feat(STONEINTG-781): remove finalizer sooner for integration plr Mar 19, 2024
helpers/integration.go Outdated Show resolved Hide resolved
@dirgim
Copy link
Collaborator

dirgim commented Mar 19, 2024

Note that the diagram for the statusreport controller will also need to be updated, at least around this part.

helpers/integration.go Outdated Show resolved Hide resolved
@Josh-Everett Josh-Everett force-pushed the 781 branch 2 times, most recently from fe930f1 to ac36a4e Compare April 9, 2024 03:14
@Josh-Everett
Copy link
Contributor Author

/retest

Copy link
Collaborator

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

The current solution looks good to me code-wise, just have a tiny nit on the diagram update.

@Josh-Everett Josh-Everett force-pushed the 781 branch 3 times, most recently from 70c9800 to 50536d9 Compare April 11, 2024 06:36
Copy link

codecov bot commented Apr 11, 2024

Codecov Report

Attention: Patch coverage is 29.16667% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 72.36%. Comparing base (0128956) to head (d8cc291).
Report is 29 commits behind head on main.

Files Patch % Lines
controllers/statusreport/statusreport_adapter.go 10.52% 14 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #626      +/-   ##
==========================================
- Coverage   72.74%   72.36%   -0.39%     
==========================================
  Files          46       46              
  Lines        4590     4657      +67     
==========================================
+ Hits         3339     3370      +31     
- Misses        900      931      +31     
- Partials      351      356       +5     

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

Copy link
Contributor

@hongweiliu17 hongweiliu17 left a comment

Choose a reason for hiding this comment

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

LGTM

Previously integration-service was prematurely removing finalizers
from "optional" Integration PLRs since they aren't considered
while marking the snapshot as finished.
The logic for these PLRS have been changed to remove their
finalizers after they are finished, their status has been recorded
by the snapshot, and their status has reported to GH.

Signed-off-by: Josh Everett <[email protected]>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@dirgim dirgim self-requested a review April 17, 2024 10:57
Copy link
Collaborator

@dirgim dirgim left a comment

Choose a reason for hiding this comment

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

Still LGTM after the latest updates

@openshift-ci openshift-ci bot added the lgtm label Apr 17, 2024
@Josh-Everett
Copy link
Contributor Author

Update to d8cc291 was just a rebase so I think this is ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants