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

ccruntime: processCcRuntimeDeleteRequest split #415

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

beraldoleal
Copy link
Member

@beraldoleal beraldoleal commented Aug 14, 2024

We are currently experiencing issues with finalizers hanging when deleting ccruntime. Initial debugging has pinpointed the problem to the processCcRuntimeDeleteRequest method.

This method is large and has a cyclomatic complexity of 21. I'm taking this opportunity to use early returns to reduce nesting and simplify control flow. Additionally, this is moving the finalizer handling logic to its own method.

This refactor should not change the current logic, only improve readability and maintainability.

Related to #391.

Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @beraldoleal !

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

It does look way better now, I noticed 2 little differences, not sure whether they are actually intended and important.

We are currently experiencing issues with finalizers hanging when
deleting ccruntime. Initial debugging has pinpointed the problem to the
processCcRuntimeDeleteRequest method.

This method is large and has a cyclomatic complexity of 21. We should
take this opportunity to use early returns to reduce nesting and
simplify control flow. Additionally, this is moving the finalizer
handling logic to its own method.

This refactor should not change the current logic, only improve
readability and maintainability.

Related to confidential-containers#391.

Signed-off-by: Beraldo Leal <[email protected]>
@beraldoleal
Copy link
Member Author

It does look way better now, I noticed 2 little differences, not sure whether they are actually intended and important.

Those were good catches. Thanks. Let me know if you are ok with the changes.

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thanks, now it seems to be doing the same thing to me except it's better readable now :-)

@ldoktor ldoktor requested a review from wainersm August 21, 2024 09:14
@beraldoleal
Copy link
Member Author

@wainersm do you mind reviewing again, since I force pushed with some changes?

@bpradipt bpradipt closed this Nov 5, 2024
@bpradipt bpradipt reopened this Nov 5, 2024
@bpradipt bpradipt requested a review from a team as a code owner November 5, 2024 16:07
@bpradipt
Copy link
Member

bpradipt commented Nov 5, 2024

@wainersm do you mind reviewing again, since I force pushed with some changes?

ping @wainersm

@bpradipt
Copy link
Member

bpradipt commented Nov 6, 2024

@stevenhorsman @fidencio can one of you trigger the e2e CI. Looks like it's stuck.

@stevenhorsman
Copy link
Member

@stevenhorsman @fidencio can one of you trigger the e2e CI. Looks like it's stuck.

You need the ok-to-test label to run all the CI.

@bpradipt
Copy link
Member

bpradipt commented Nov 6, 2024

@stevenhorsman @fidencio can one of you trigger the e2e CI. Looks like it's stuck.

You need the ok-to-test label to run all the CI.

Oh ok..

@bpradipt
Copy link
Member

bpradipt commented Nov 6, 2024

Merging this as CI passed successfully.

@bpradipt bpradipt merged commit 0e55097 into confidential-containers:main Nov 6, 2024
24 checks passed
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.

5 participants