-
Notifications
You must be signed in to change notification settings - Fork 70
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 unsupported images via shell in our e2e tests #1541
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
…ication CSI' Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
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.
Wow. Just Amazing.
@@ -155,13 +150,16 @@ func (v *DpaCustomResource) CreateOrUpdate(c client.Client, spec *oadpv1alpha1.D | |||
}, | |||
Spec: *spec.DeepCopy(), | |||
} | |||
return v.Create(dpa) |
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.
All changes from this line on, I think are unnecessary
the workflow is: UnsupportedOverrides will be in json, tests/e2e/e2e_suite_test.go
will load the values, line 96 of this file will add values to DPAs created by test run
changes being done on this line on are duplication of was already done in line 96
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.
You are incorrect. There is duplication in how the tests create the dpa. My addition requires the above. I was also confused about that. DPA creation is handled in two different ways.
Follow the dpa creation from the two points:
- make test-e2e GINKGO_ARGS="--ginkgo.focus='MySQL application DATAMOVER'"
- make test-e2e GINKGO_ARGS="--ginkgo.focus='DPA reconciled to true'"
Should we perhaps fix that yes, but it's certainly NOT in scope for this change :) 1 scope per PR.
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.
Understood what you want to do, but I do not think this is the correct approach
Since tests here https://github.com/openshift/oadp-operator/blob/d5adc852931f1e23a24c7f0be7f86a27d107ce2d/tests/e2e/dpa_deployment_suite_test.go do not use DpaCustomResource.Build
function, you were not seeing UnsupportedOverrides
there
But this change breaks this test
oadp-operator/tests/e2e/dpa_deployment_suite_test.go
Lines 362 to 369 in d5adc85
ginkgo.Entry("DPA CR with unsupportedOverrides", ginkgo.Label("aws", "ibmcloud"), InstallCase{ | |
DpaSpec: createTestDPASpec(TestDPASpec{ | |
BSLSecretName: bslSecretName, | |
UnsupportedOverrides: map[oadpv1alpha1.UnsupportedImageKey]string{ | |
"awsPluginImageFqin": "quay.io/konveyor/velero-plugin-for-aws:oadp-1.4", | |
}, | |
}), | |
}), |
If I export AWS_PLUGIN_IMAGE
, or not, the value defined in json will overwrite value defined in 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.
The way I would go with this:
- remove the test mentioned above (since the idea is to run these tests with values of
UnsupportedOverrides
in the near future, so test would not be necessary anymore) - remove this
UnsupportedOverrides map[oadpv1alpha1.UnsupportedImageKey]string - change
testSpec.UnsupportedOverrides
here todpaCR.UnsupportedOverrides
UnsupportedOverrides: testSpec.UnsupportedOverrides,
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.
ok.. thank you for the in depth review! I think I have made the changes you requested, I'm going to push an update and take some time to double check the changes and 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.
with the current changes, I think you can safely undo changes in lines 156-158 and line 165
the same changes are being applied to line 68 of tests/e2e/dpa_deployment_suite_test.go
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weshayutin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: weshayutin The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks, addressed all the comments sir!
/retest |
* removed image override test * remove unsed var declaration * other mindor changes Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
Signed-off-by: Wesley Hayutin <weshayutin@gmail.com>
@weshayutin: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Why the changes were made
Generally speaking this will be handy for dev's to be able to easily use custom images in our e2e tests.
This especially is handy as we build out and use openshift/oadp-operator e2e tests across our various repos.
How to test the changes made
Each cloud provider has a json template for the dpa under tests/e2e/scripts. All the unsupported Image Overrides were added to each provider. If the variable is empty oadp will use the default images [1]. The map of strings is patched to the created dpa.
[1]