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

allow test utils be extended #1520

Merged
merged 1 commit into from
May 29, 2020
Merged

allow test utils be extended #1520

merged 1 commit into from
May 29, 2020

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented May 15, 2020 •
edited
Loading

Description
Allow the test utils to be used for other projects as well.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86

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 requested review from droot and estroz May 15, 2020 18:46
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2020
@camilamacedo86 camilamacedo86 changed the title allow util tests be extended allow test utils be extended May 15, 2020
estroz
estroz suggested changes May 15, 2020
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Two suggestions.

test/e2e/utils/test_context.go Outdated Show resolved Hide resolved
test/e2e/utils/util.go Outdated Show resolved Hide resolved
@camilamacedo86 camilamacedo86 changed the title allow test utils be extended WIP: allow test utils be extended May 15, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2020
@estroz
Copy link
Contributor

estroz commented May 15, 2020

These modifications to e2e utils will be beneficial to any plugin developers, since they expose wrappers for init, create api, and create webhook.

/cc @DirectXMan12 @droot @mengqiy

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 15, 2020
@camilamacedo86 camilamacedo86 force-pushed the allow-test-utils branch 2 times, most recently from a9ecfbc to d88b5a7 Compare May 15, 2020 19:36
@camilamacedo86 camilamacedo86 changed the title WIP: allow test utils be extended allow test utils be extended May 15, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2020
@camilamacedo86
Copy link
Member Author

/hold

Until get OK from 2 reviewers.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2020
@camilamacedo86
Copy link
Member Author

c/c @joelanford

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-16-2

@camilamacedo86
Copy link
Member Author

/hold cancel
(received review of 2 persons and suggestions are addressed already)

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2020
@estroz estroz mentioned this pull request May 26, 2020
Merged
mengqiy
mengqiy reviewed May 29, 2020
View reviewed changes
Copy link
Member

@mengqiy mengqiy left a comment

Choose a reason for hiding this comment

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

A few nits.
Otherwise LGTM
To be very clear, there is no backward compatibility guarantee for these util APIs.

testdata/project-v2-addon/go.mod Outdated
@@ -9,5 +9,5 @@ require (
k8s.io/apimachinery v0.18.2
k8s.io/client-go v0.18.2
sigs.k8s.io/controller-runtime v0.6.0
sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200522144838-848d48e5b073
Copy link
Member

Choose a reason for hiding this comment

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

Is this change really needed in this PR?

Copy link
Member Author

@camilamacedo86 camilamacedo86 May 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

solved here: #1532 - It is related to the scenario that we need to solve to avoid it. See; #1527

testdata/project-v3-addon/go.mod Outdated
@@ -9,5 +9,5 @@ require (
k8s.io/apimachinery v0.18.2
k8s.io/client-go v0.18.2
sigs.k8s.io/controller-runtime v0.6.0
sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200512162422-ce639cbf6d4c
sigs.k8s.io/kubebuilder-declarative-pattern v0.0.0-20200522144838-848d48e5b073
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto.

@camilamacedo86
Copy link
Member Author

camilamacedo86 commented May 29, 2020 •
edited
Loading

The nit is solved. So, all ok to /lgtm this one.

@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-16-2

estroz
estroz approved these changes May 29, 2020
View reviewed changes
Copy link
Contributor

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 2525aeb into kubernetes-sigs:master May 29, 2020
@camilamacedo86 camilamacedo86 mentioned this pull request Feb 18, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estroz estroz estroz approved these changes

@droot droot Awaiting requested review from droot

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@hasbro17 hasbro17 Awaiting requested review from hasbro17

@mengqiy mengqiy Awaiting requested review from mengqiy

Assignees

@estroz estroz

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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@camilamacedo86 @k8s-ci-robot @estroz @mengqiy @hasbro17