-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 delete resources #1915
🏃E2e delete resources #1915
Conversation
df1bb00
to
8d2ad78
Compare
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'm inclined to agree that we shouldn't add a separate delete test but we should be testing for cleanup after every (at least for now) test. This should keep the time the test suite runs to a minimum while still exercising the delete code written here.
8d2ad78
to
57add85
Compare
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 really like the consolidation but I want to keep the framework provider agnostic. The providers should be maintaining their provider specific code outside of this framework
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.
looks great!
/approve
Just need to get the tests passing
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha, wfernandes 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 |
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.
Minor nit, otherwise lgtm
174c554
to
e6c8186
Compare
/retest |
lgtm, but should probably squash some of these commits before we merge. |
test/framework/go.mod
Outdated
sigs.k8s.io/cluster-api v0.2.6-0.20191216153338-db9baf9ade0c | ||
sigs.k8s.io/controller-runtime v0.4.0 | ||
) | ||
|
||
replace ( | ||
k8s.io/client-go => k8s.io/client-go v0.0.0-20190918160344-1fbdaa4c8d90 |
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.
What replaces this directive? Kind?
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.
TBH I'm not sure I understand the question. I added the replace directive to match the version in cluster-api and test/infrastructure/docker.
Signed-off-by: Warren Fernandes <[email protected]>
Signed-off-by: Warren Fernandes <[email protected]>
Update ginkgo/gomega dependencies Remove usage of OneNodeClusterInput Bump test/e2e module to go 1.13 Refactor MultiNodeControlplaneClusterInput Refactor CleanUp to assert artifacts are deleted Signed-off-by: Warren Fernandes <[email protected]>
Signed-off-by: Warren Fernandes <[email protected]>
Signed-off-by: Warren Fernandes <[email protected]>
Signed-off-by: Warren Fernandes <[email protected]>
315cc44
to
97725fa
Compare
@detiber I squashed some of the commits to reduce noise but still keep the independent ones separate. Let me know if this looks good or if you'd like me to squash em all the way down to one. |
0d7f075
to
25441a7
Compare
Thanks for addressing the major concerns. I'm fine merging this and refactoring anything that isn't working out. /assign @detiber for final lgtm. I believe the policy is "one PR for a big change but many commits is fine". please correct me if I've misunderstood |
lgtm, however it looks like the verify job is failing |
Yup...when I run |
Signed-off-by: Warren Fernandes <[email protected]>
25441a7
to
65df9d9
Compare
/lgtm |
What this PR does / why we need it:
DeleteCluster
test to the e2e test framework.Created multiple commits for easier review process.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Ref #1732