-
Notifications
You must be signed in to change notification settings - Fork 60
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: Access config and clusters via test context #1916
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
f39677e
to
c43eb57
Compare
nirs
commented
Mar 10, 2025
9ff50a5
to
e553f74
Compare
parikshithb
reviewed
Mar 11, 2025
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.
Changes looks great!
Changes in latest version:
|
parikshithb
approved these changes
Mar 13, 2025
Signed-off-by: Nir Soffer <[email protected]>
There is no reason to make the command line options package globals since they are consumed only in TestMain(). Signed-off-by: Nir Soffer <[email protected]>
We used to keep the main logger in utils.Ctx.Log and access it from all code that does not run in test context. This does not work for ramenctl which want to create the logger and pass it to commands as parameter. Since we need to keep global test variables, add a Context type to e2e_test package. This will be the only global variable to make managed of global variables easier and explicit. Move the logger from util.Ctx.Log to Ctx.log. This makes the logger accessible only the tests in the e2e_test package. The tests pass this log around to other code, or add it to the per-test context. Signed-off-by: Nir Soffer <[email protected]>
- Import "k8s.io/apimachinery/pkg/api/errors" as k8serrors - Import "k8s.io/apimachinery/pkg/types" as k8types This make room for "github.com/ramendr/ramen/e2e/types" and standard library "errors" package. Signed-off-by: Nir Soffer <[email protected]>
We want to keep the config in the context, and for this we need to provide and interface returning a cluster. So the cluster must be part of the types package. This also remove bad dependency on the util package, preventing import of the types package in the util package. Signed-off-by: Nir Soffer <[email protected]>
Consistent import order: - standard library - 3rp party imports - e2e imports Signed-off-by: Nir Soffer <[email protected]>
The environment is important concept so it should have it own package. The new Env type replaces util.Context. We create the instance in main_test.go, and pass it to the tests via package global. The dr test add the env to the test context, and most of the code access the clusters via the ctx.Env(). Signed-off-by: Nir Soffer <[email protected]>
We want to access the config from types.Context. To do this without introducing complex dependencies, the types should be part of the types package. Signed-off-by: Nir Soffer <[email protected]>
All validation is done now in a validate* function returning an error. This simplifies the function, removing the linter skip, makes the code easier to follow, and make it easier to extend validation for parts we don't validate properly (PVCSpecs). Signed-off-by: Nir Soffer <[email protected]>
Functions are easier to follow when using the same abstraction level. Move out the code reading the config to a helper to make ReadConfig() very simple and clear. Signed-off-by: Nir Soffer <[email protected]>
When we added config.Namespaces it was never used. Change validateDistro to also initialize the Namespaces field. Signed-off-by: Nir Soffer <[email protected]>
Previously we kept the single config in the config package, and access it using functions. Now we create a config object and pass it to the tests via the global context. The test pass the config to code operating on the global context (e.g. create a channel), and pass the config to the test context. The rest of the code access the config using the test context, in same way we access the env and log. This eliminate the dependency on the config package. It is used now only in main_test.go and in dr_test.go for getting PVCSpecs mapping. In ramenctl will use this to create a config object and pass it to the command as parameter. Signed-off-by: Nir Soffer <[email protected]>
raghavendra-talur
approved these changes
Mar 16, 2025
This was referenced Mar 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently the config is stored in the config package, and accessed using package accessors. This is needed since we don't have a way to pass the config from TestMain() to other tests, but it does not work for ramenctl, that want to parse the config and pass it as parameter.
Currently we access the clusters via utils.Ctx global. This will not work for ramenctl that need to create the clusters for all commands and pass them as parameters.
Currently we access the main logger via utils.Ctx global. This wil not work for ramenctl that need to create a logger and pass it to the test context.
Changes:
Keep the main logger in test package global. The test pass the log the log to the test context or to utils functions that need the main logger.
Extract e2e/env package providing the Env type, holding the clusters. We create an Env in TestMain() and pass it to the test using test package global. Tests pass the env to the test context. e2e code access the cluster via the context.
config.ReadConfig() returns a config object. The config is passed to the tests using test package global. The tests will pass the config to the text context, and all e2e code access the config via the context.
In ramenctl we will create a Config and Env and pass them as parameters.
Example ramenctl usage:
RamenDR/ramenctl#33