-
Notifications
You must be signed in to change notification settings - Fork 58
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
e2e: adding testcases with recipe #1736
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Annaraya Narasagond <[email protected]>
98cec44
to
6ab815a
Compare
Signed-off-by: Annaraya Narasagond <[email protected]>
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.
Recipe is part of the workload, not a way to deploy the workload.
I think we can add options for workload to create a recipe with optional hook, and a way to select these workloads only for discoveredapps deployer. For example we can have IsDiscovered() for workload which will make it run only with discovered apps deployer.
} | ||
|
||
log.Info("recipe created on both dr clusters") | ||
} |
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.
This should be part of the workload, not of the deployer.
if err := deleteRecipe(util.Ctx.C2.Client, recipeName, appNamespace); err != nil { | ||
return err | ||
} | ||
} |
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.
Same, part of the workload.
|
||
return nil | ||
} | ||
|
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.
functions for creating recipes should move to recipes package or maybe workloads since recipes are part of a workload.
e2e/exhaustive_suite_test.go
Outdated
appset = &deployers.ApplicationSet{} | ||
discoveredApps = &deployers.DiscoveredApp{} | ||
discoveredAppsWithoutHook = &deployers.DiscoveredApp{IncludeRecipe: true, IncludeHooks: false} | ||
discoveredAppsWithHook = &deployers.DiscoveredApp{IncludeRecipe: true, IncludeHooks: true} |
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.
I think we need to keep discoveredApps deployer and add workloads including a recipe with/without hooks.
Also we don't want to add so many tests - this add 2 new deployers, which adds 4 new tests. Why do w need to test recipe without hook and recipe with hook with both rbd and ceph storage? Does recipe/hooks care about the storage which is handled by ramen anyway?
if err := recipe.AddToScheme(scheme); err != nil { | ||
return err | ||
} | ||
|
||
return ramen.AddToScheme(scheme) | ||
} | ||
|
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.
Lets split the part adding recipe support to another commit or PR. This is preparation for adding recipes.
Fixes #1698 partially which adds following cases: