-
Notifications
You must be signed in to change notification settings - Fork 32
feat: Introduce Mandatory Module Deletion Service #2818
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
base: main
Are you sure you want to change the base?
feat: Introduce Mandatory Module Deletion Service #2818
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some comments left :)
"github.com/kyma-project/lifecycle-manager/api/v1beta2" | ||
) | ||
|
||
type UseCase interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But not so sure if the ShouldExecute
is needed.
You could just Execute
the usecase and return the same errors or nil as Should Execute
. Feels like a unnecessary indirection.
Usecases are anyways not very complex and mostly pass through to repos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can talk about it in detail tomorrow in the office. @LeelaChacha is coming as well.
But the basic idea was that per invocation of the reconciliation loop, only one use case is executed. E.g., if ensureFinalizer
should execute, we will apply the finalizer and return to the controller. Only in the next invocation of the reconciliation loop another use case may execute consecutively.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:) No need, I got the idea. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then just as a small improvement suggestion, let's rename to the following:
// IsApplicable checks if this deletion step use-case should be executed now
IsApplicable(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error)
"github.com/kyma-project/lifecycle-manager/api/v1beta2" | ||
) | ||
|
||
type UseCase interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then just as a small improvement suggestion, let's rename to the following:
// IsApplicable checks if this deletion step use-case should be executed now
IsApplicable(ctx context.Context, mrm *v1beta2.ModuleReleaseMeta) (bool, error)
Co-authored-by: Benjamin Lindner <[email protected]>
Description
Changes proposed in this pull request:
Related issue(s)
#2631