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.

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

Add more tests for the pod disruption budget endpoints #85819

Open
wants to merge 6 commits into
base: master
from

Conversation

@nan-yu
Copy link
Contributor

nan-yu commented Dec 2, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Add more coverage for the pod disruption budget e2e test

Special notes for your reviewer:
Last e2e test coverage report: #84740 (comment)

Endpoint Tested Update Note
patchPolicyV1beta1NamespacedPodDisruptionBudgetStatus Untested Untested Need to add client method
readPolicyV1beta1NamespacedPodDisruptionBudgetStatus Untested Untested Need to add client method
replacePolicyV1beta1NamespacedPodDisruptionBudgetStatus Untested Untested Does it make sense to update the status without updating PDB?
listPolicyV1beta1PodDisruptionBudgetForAllNamespaces Untested Tested
deletePolicyV1beta1CollectionNamespacedPodDisruptionBudget Untested Tested
patchPolicyV1beta1NamespacedPodDisruptionBudget Untested Tested
listPolicyV1beta1NamespacedPodDisruptionBudget Untested Tested
replacePolicyV1beta1NamespacedPodDisruptionBudget Tested Tested
deletePolicyV1beta1NamespacedPodDisruptionBudget Tested Tested
createPolicyV1beta1NamespacedPodDisruptionBudget Tested Tested
readPolicyV1beta1NamespacedPodDisruptionBudget Tested Tested

Does this PR introduce a user-facing change?:

NONE
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

@nan-yu: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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/test-infra repository.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

Welcome @nan-yu!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

Hi @nan-yu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

@nan-yu

This comment has been minimized.

Copy link
Contributor Author

nan-yu commented Dec 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nan-yu
To complete the pull request process, please assign fabriziopandini, kow3ns
You can assign the PR to them by writing /assign @fabriziopandini @kow3ns in a comment when ready.

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

@k8s-ci-robot k8s-ci-robot requested review from soltysh and timothysc Dec 2, 2019
@mortent

This comment has been minimized.

Copy link
Member

mortent commented Dec 2, 2019

/ok-to-test

@nan-yu nan-yu changed the title Add more tests for the ../poddisruptionbudgets/{name} endpoint Add more tests for the pod disruption budget endpoints Dec 2, 2019

appsv1 "k8s.io/api/apps/v1"
"k8s.io/api/core/v1"
v1 "k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@mortent

mortent Dec 2, 2019

Member

corev1 is a better name here.

This comment has been minimized.

Copy link
@nan-yu

nan-yu Dec 2, 2019

Author Contributor

corev1 violates the import-aliases rule 😞

ERROR wrong alias for import "k8s.io/api/core/v1" should be v1 in file test/e2e/apps/disruption.go

"github.com/onsi/ginkgo"
"github.com/onsi/gomega"
"k8s.io/apimachinery/pkg/types"

This comment has been minimized.

Copy link
@mortent

mortent Dec 2, 2019

Member

Move the two k8s imports to the next block.

@k8s-ci-robot k8s-ci-robot added kind/cleanup and removed needs-kind labels Dec 2, 2019
@nan-yu nan-yu force-pushed the nan-yu:pdb_e2etest branch from f8d6676 to b8b5a90 Dec 2, 2019
@nan-yu nan-yu force-pushed the nan-yu:pdb_e2etest branch from b8b5a90 to 4f07fb3 Dec 2, 2019
@RA489

This comment has been minimized.

Copy link
Contributor

RA489 commented Dec 3, 2019

/release-note-none

@RA489

This comment has been minimized.

Copy link
Contributor

RA489 commented Dec 3, 2019

/release-note-none

@RA489

This comment has been minimized.

Copy link
Contributor

RA489 commented Dec 3, 2019

/retest

@mortent
mortent approved these changes Dec 3, 2019
Copy link
Member

mortent left a comment

A small comment, but this looks good to me.
/lgtm

framework.ExpectNoError(err) // the eviction is now allowed
})

ginkgo.Context("With another framework", func() {

This comment has been minimized.

Copy link
@mortent

mortent Dec 3, 2019

Member

Can we change the description here to reflect what we are testing rather than describing how we are doing it? I think what we are checking here is that we can create and list PDBs from different namespaces.

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 3, 2019
@nan-yu nan-yu force-pushed the nan-yu:pdb_e2etest branch from 4f07fb3 to a205bfd Dec 3, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 3, 2019
@nan-yu

This comment has been minimized.

Copy link
Contributor Author

nan-yu commented Dec 3, 2019

/test pull-kubernetes-e2e-kind

@nan-yu

This comment has been minimized.

Copy link
Contributor Author

nan-yu commented Dec 4, 2019

/test pull-kubernetes-integration

@mortent
mortent approved these changes Dec 4, 2019
Copy link
Member

mortent left a comment

/lgtm


ginkgo.It("should list and delete a collection of PodDisruptionBudgets", func() {
specialLabels := map[string]string{"foo_pdb": "bar_pdb"}
labelSelector := "foo_pdb=bar_pdb"

This comment has been minimized.

Copy link
@mortent

mortent Dec 4, 2019

Member

You can use labels.SelectorFromSet here to turn the labels into a selector.

This comment has been minimized.

Copy link
@nan-yu

nan-yu Dec 5, 2019

Author Contributor

Good to know. Done!

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 4, 2019
@mortent

This comment has been minimized.

Copy link
Member

mortent commented Dec 4, 2019

nan-yu added 6 commits Nov 27, 2019
/apis/policy/v1beta1/namespaces/{namespace}/poddisruptionbudgets
- DELETE: deletePolicyV1beta1CollectionNamespacedPodDisruptionBudget: delete collection of PodDisruptionBudget
- GET: listPolicyV1beta1NamespacedPodDisruptionBudget: list or watch objects of kind PodDisruptionBudget
The last commit adds a test to list all PDBs across namespaces and
validates the names of PDBs.
However, the test cases are running in random order, which might
have some leftover PDBs in namespaces.
This commit adds a special label in the created PDBs to avoid name
conflict.
@nan-yu nan-yu force-pushed the nan-yu:pdb_e2etest branch from ca54709 to 8987442 Dec 5, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm label Dec 5, 2019
@mortent
mortent approved these changes Dec 5, 2019
Copy link
Member

mortent left a comment

/lgtm
You should probably squash the commits before merge.

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 5, 2019
@nan-yu

This comment has been minimized.

Copy link
Contributor Author

nan-yu commented Dec 5, 2019

/test pull-kubernetes-conformance-kind-ipv6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.