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.
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?
Set unsupported images via shell in our e2e tests #1541
Changes from all commits
9f868a4
6a7812c
a832745
0f26958
b2d9dd2
f96662e
80579c4
131b389
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 runchanges 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:
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 seeingUnsupportedOverrides
thereBut this change breaks this test
oadp-operator/tests/e2e/dpa_deployment_suite_test.go
Lines 362 to 369 in d5adc85
If I export
AWS_PLUGIN_IMAGE
, or not, the value defined in json will overwrite value defined in testThere 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:
UnsupportedOverrides
in the near future, so test would not be necessary anymore)oadp-operator/tests/e2e/dpa_deployment_suite_test.go
Line 35 in d5adc85
testSpec.UnsupportedOverrides
here todpaCR.UnsupportedOverrides
oadp-operator/tests/e2e/dpa_deployment_suite_test.go
Line 69 in d5adc85
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