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

[addon-operator] add sdk and batch hooks #529

Merged
merged 15 commits into from
Dec 4, 2024
Merged

[addon-operator] add sdk and batch hooks #529

merged 15 commits into from
Dec 4, 2024

Conversation

ldmonster
Copy link
Contributor

@ldmonster ldmonster commented Nov 13, 2024

Overview

Refactoring of global values
Replace snake_case names
Bump shell-operator
Add new module type (now we have EmbeddedModule and Module)
Add interface PatchCollector
Add interface TaskMetadata

Add Batch Hook logic

What this PR does / why we need it

Special notes for your reviewer

@ldmonster ldmonster added the dependencies Pull requests that update a dependency file label Nov 13, 2024
@ldmonster ldmonster self-assigned this Nov 13, 2024
@ldmonster ldmonster force-pushed the feature/add-sdk branch 3 times, most recently from 5714cce to 60c9f87 Compare November 15, 2024 13:58
@ldmonster ldmonster changed the title [addon-operator] feat/add sdk [addon-operator] refactoring Nov 15, 2024
@ldmonster ldmonster marked this pull request as ready for review November 15, 2024 14:26
@ldmonster ldmonster requested a review from yalosev November 15, 2024 14:26
@ldmonster ldmonster added enhancement New feature or request and removed dependencies Pull requests that update a dependency file labels Nov 15, 2024
@yalosev yalosev requested a review from miklezzzz November 19, 2024 12:00
Copy link
Contributor

@miklezzzz miklezzzz left a comment

Choose a reason for hiding this comment

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

and a global question - what was wrong about using Public variables from the App package comparing to forwarding the same values via arguments to functions/methods?

pkg/module_manager/models/hooks/kind/shellhook.go Outdated Show resolved Hide resolved
pkg/module_manager/models/hooks/kind/shellhook.go Outdated Show resolved Hide resolved
pkg/module_manager/models/hooks/kind/shellhook.go Outdated Show resolved Hide resolved
@miklezzzz miklezzzz self-requested a review November 20, 2024 15:11
@ldmonster
Copy link
Contributor Author

and a global question - what was wrong about using Public variables from the App package comparing to forwarding the same values via arguments to functions/methods?

Global variables are appropriate when we use them as a singleton (IMHO any singleton is a design error, and we should strive to exclude it from the code base). In our case, global vars are used only to read flags. Due to the fact that we directly throw them inside our functions, we have high coupling.
To make the code more readable and easy to change, in this PR I partially isolated global variables from implementations.

@ldmonster ldmonster changed the title [addon-operator] refactoring [addon-operator] add sdk Dec 4, 2024
Signed-off-by: Pavel Okhlopkov <[email protected]>
Pavel Okhlopkov added 9 commits December 4, 2024 14:12
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Pavel Okhlopkov added 3 commits December 4, 2024 14:59
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
Signed-off-by: Pavel Okhlopkov <[email protected]>
@yalosev yalosev merged commit b6e8ee2 into main Dec 4, 2024
8 checks passed
@yalosev yalosev changed the title [addon-operator] add sdk [addon-operator] add sdk and batch hooks Dec 4, 2024
@yalosev yalosev deleted the feature/add-sdk branch December 4, 2024 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants