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

Conformance: Add StatefulSet tests. #60336

Merged
merged 1 commit into from Feb 27, 2018

Conversation

@enisoc
Copy link
Member

enisoc commented Feb 24, 2018

Mark StatefulSet tests as Conformance where possible. I've excluded those that depend on a dynamic provisioner and a default storage class (i.e. those that use PVC), because I don't think those things are required for Conformance at this time.

@kow3ns @jagosan Please correct me if I'm wrong.

Part of #54256

StatefulSet in apps/v1 is now included in Conformance Tests.
@@ -901,21 +913,29 @@ var _ = SIGDescribe("StatefulSet", func() {
framework.DeleteAllStatefulSets(c, ns)
})

// This can't be Conformance yet because it depends on a default

This comment has been minimized.

@kow3ns

kow3ns Feb 24, 2018

Member

This should never be conformance. We should consider removing it. The same is true for all of the application specific tests that follow it. We haven't removed them because they have been improved to the point that they provide a signal, but we shouldn't require them for API conformance.

This comment has been minimized.

@bgrant0607

bgrant0607 Feb 26, 2018

Member

Is it because they are application-specific, or because they rely on dynamic volume provisioning, which is an optional capability?

This comment has been minimized.

@enisoc

enisoc Feb 26, 2018

Author Member

Ken's comment was originally on the ZooKeeper example, and he was referring only to the app-specific tests. I rewrote the comments on those to explain that they are excluded for being app-specific.

After the squash and force push, GitHub has mistakenly placed his comment on a totally different line (that happens to have the same contents as the line he previously commented on).

The "should provide basic identity" test was never in question for being app-specific. It is only excluded because it relies on dynamic provisioning and a default storage class, as indicated in the comment.

@kow3ns
Copy link
Member

kow3ns left a comment

Generally looks good and is the set I would want for conformance except for the exceptions listed below.

@@ -242,7 +248,8 @@ var _ = SIGDescribe("StatefulSet", func() {
sst.WaitForRunningAndReady(*ss.Spec.Replicas, ss)
})

It("should perform rolling updates and roll backs of template modifications", func() {
// This is ok for Conformance because it doesn't use any PVCs.
framework.ConformanceIt("should perform rolling updates and roll backs of template modifications", func() {

This comment has been minimized.

@kow3ns

kow3ns Feb 24, 2018

Member

You need to add the appropriate conformance comment. See the README in the conformance dir. The same goes for all selected conformance tests.

@@ -566,7 +574,8 @@ var _ = SIGDescribe("StatefulSet", func() {

})

It("should implement legacy replacement when the update strategy is OnDelete", func() {
// This is ok for Conformance because it doesn't use any PVCs.
framework.ConformanceIt("should implement legacy replacement when the update strategy is OnDelete", func() {

This comment has been minimized.

@kow3ns

kow3ns Feb 24, 2018

Member

Don't make this conformance. It's not clear that OnDelete will continue to be a supported update strategy after extensions/v1beta1 is removed.

@enisoc enisoc force-pushed the enisoc:statefulset-conformance branch from ca49717 to 9b266e9 Feb 26, 2018

@enisoc enisoc force-pushed the enisoc:statefulset-conformance branch from 9b266e9 to 1e82d13 Feb 26, 2018

@enisoc

This comment has been minimized.

Copy link
Member Author

enisoc commented Feb 26, 2018

@kow3ns Comments addressed. PTAL.

@kow3ns

kow3ns approved these changes Feb 26, 2018

@kow3ns

This comment has been minimized.

Copy link
Member

kow3ns commented Feb 26, 2018

/lgtm

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Feb 26, 2018

LGTM from a conformance perspective. It looks like the "This can't be Conformance yet because..." comments still need some rewording.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Feb 26, 2018

@enisoc

This comment has been minimized.

Copy link
Member Author

enisoc commented Feb 26, 2018

@bgrant0607 GitHub moved @kow3ns' comment around, which made it look unresolved. In reality, all his comments have been resolved.

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Feb 26, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgrant0607, enisoc, kow3ns

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

@enisoc enisoc added this to the v1.10 milestone Feb 26, 2018

@kow3ns kow3ns added this to In Progress in Workloads Feb 26, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Feb 27, 2018

Automatic merge from submit-queue (batch tested with PRs 59674, 60059, 60220, 58916, 60336). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b64230b into kubernetes:master Feb 27, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation enisoc authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

Workloads automation moved this from In Progress to Done Feb 27, 2018

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.