Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨ [e2e framework]: add skip-resource-cleanup flag to allow skipping any clean up #2296

Merged
merged 1 commit into from
Feb 18, 2020
Merged

✨ [e2e framework]: add skip-resource-cleanup flag to allow skipping any clean up #2296

merged 1 commit into from
Feb 18, 2020

Conversation

SataQiu
Copy link
Member

@SataQiu SataQiu commented Feb 9, 2020 •
edited
Loading

What this PR does / why we need it:
Feature: add -skip-resource-cleanup flag to allow skipping any clean up
Now we can run e2e test like this to skip cleanup:

go test ./e2e -v -ginkgo.v -ginkgo.trace -count=1 -timeout=20m -tags=e2e -e2e.config="$(abspath $(E2E_CONF_FILE))" -skip-resource-cleanup

or

make run-e2e SKIP_RESOURCE_CLEANUP=true

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2268

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 9, 2020
@k8s-ci-robot k8s-ci-robot requested review from justinsb and ncdc February 9, 2020 09:05
fabriziopandini
fabriziopandini reviewed Feb 10, 2020
View reviewed changes
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have two concern about this PR, mostly for lack of deep knowledge of this part of the codebase:

  • Is there a way to make the new flag accessible via the Make run-e2e target?
  • Is it possible to make mgmt.Teardown and ensureDockerArtifactsDeleted to behave according to the flag instead of wrapping those calls into an if statement?

@chuckha chuckha self-assigned this Feb 10, 2020
@chuckha
Copy link
Contributor

chuckha commented Feb 10, 2020

I agree with @fabriziopandini's concerns. However, @SataQiu, take a look here #2294 It might be easier to implement a skip when this PR lands

@chuckha
Copy link
Contributor

chuckha commented Feb 14, 2020

Would love to get this feature in. Do you think the refactor has helped ease the implementation here at all?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2020
@SataQiu
Copy link
Member Author

SataQiu commented Feb 15, 2020

Would love to get this feature in. Do you think the refactor has helped ease the implementation here at all?

The refactor is very good! Some checks can be done within the framework. 👍

@SataQiu SataQiu changed the title ✨ [e2e framework]: add -skip-cleanup flag to allow skipping any clean up ✨ [e2e framework]: add skip-resource-cleanup flag to allow skipping any clean up Feb 15, 2020
SataQiu
SataQiu commented Feb 15, 2020
View reviewed changes
test/framework/options/generic.go
@@ -0,0 +1,29 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add options package to avoid import cycle

@SataQiu
Copy link
Member Author

SataQiu commented Feb 16, 2020

/test pull-cluster-api-capd-e2e

@vincepri
Copy link
Member

/milestone v0.3.0

@k8s-ci-robot k8s-ci-robot added this to the v0.3.0 milestone Feb 16, 2020
@chuckha
Copy link
Contributor

chuckha commented Feb 18, 2020 •
edited
Loading

Nice! Thanks for the refactor

After we use this for a bit we should see if it makes sense to add an option to the config file, but for now this is totally fine.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 18, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, SataQiu

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 722be19 into kubernetes-sigs:master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabriziopandini fabriziopandini fabriziopandini left review comments

@justinsb justinsb Awaiting requested review from justinsb

@ncdc ncdc Awaiting requested review from ncdc

Assignees

@chuckha chuckha

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

[e2e framework] Add feature to allow skipping any clean up
5 participants
@SataQiu @chuckha @vincepri @k8s-ci-robot @fabriziopandini