-
Notifications
You must be signed in to change notification settings - Fork 8
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
Set Environment Variable for Tests #49
Conversation
test/e2e/e2e_suite_test.go
Outdated
@@ -83,6 +83,8 @@ var _ = BeforeSuite(func() { | |||
k8sClient, err = ctrl.New(config, ctrl.Options{Scheme: scheme.Scheme}) | |||
Expect(err).NotTo(HaveOccurred()) | |||
|
|||
os.Setenv("DEPLOYMENT_NAMESPACE", operatorInstalledNamespcae) |
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.
Why is this needed in the e2e test? 🤔
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.
Function GetFenceAgentsRemediationPod calls function GetDeploymentNamespace which is looking for the namespace value from DEPLOYMENT_NAMESPACE envioronment variable.
Without this line I receive an error (see below) and the e2e test fails with timeout.
Error:
failed to get pod. Might try again {"error": "failed fetching FAR namespace - DEPLOYMENT_NAMESPACE must be set"}
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 why isn't it set? It should be set automatically, because CI deploys the operator the same way as we do in production 🤔
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.
nevermind, found it, it's indirectly needed by the test code as well, and not just controller code
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.
So lgtm ? :)
pkg/utils/pods.go
Outdated
@@ -24,6 +24,7 @@ func GetFenceAgentsRemediationPod(r client.Reader) (*corev1.Pod, error) { | |||
podNamespace, err := GetDeploymentNamespace() | |||
if err != nil { | |||
logger.Error(err, "failed fetching FAR namespace") | |||
return nil, 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.
This does not seem just related to the test.
Maybe a specific commit (if not a PR) to highlight this change might be beneficial
Environment variable DEPLOYMENT_NAMESPACE was missing in the UT and E2E tests
Wrapping the errors from GetFenceAgentsRemediationPod and log the error once in main or in checkFarLogs
Environment variable DEPLOYMENT_NAMESPACE was missing in the E2E tests
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7, slintes The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
A follow up PR for #42 which add an environment variable DEPLOYMENT_NAMESPACE to the
manager
container, but we need to set in the UT and E2E tests, otherwise they have raised an error (and now they will fail).