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

Split mutable and read-only access to feature gates, limit tests to readonly access #71302

Merged
merged 6 commits into from Nov 30, 2018

Conversation

@liggitt
Copy link
Member

liggitt commented Nov 21, 2018

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

Follow-up to #71100

  • Tightens the DefaultFeatureGate interface to only allow read access.
  • Moves the mutable feature gate methods into a MutableFeatureGate interface
  • Switches all remaining unit tests to using the feature gate testing package that auto-restores original values
  • Adds a verification script to ensure no test code uses DefaultMutableFeatureGate or the MutableFeatureGate interface, and that all calls to SetFeatureGateDuringTest make a deferred call the returned reset function
  • Drops the stop-gap package-level detection methods added in #71100 (funneling all test access through correct invocations of SetFeatureGateDuringTest is sufficient)

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 #71108
Fixes #71236

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The `DefaultFeatureGate` package variable now only exposes readonly feature gate methods. Methods for mutating feature gates have moved into a `MutableFeatureGate` interface and are accessible via the `DefaultMutableFeatureGate` package variable. Only top-level commands and options setup should access `DefaultMutableFeatureGate`.

@liggitt liggitt force-pushed the liggitt:verify-unit-test-feature-gates branch from 15756bc to 5196876 Nov 21, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 21, 2018

/priority important-soon
/assign @smarterclayton

@liggitt liggitt changed the title Verify unit test feature gates Split mutable and read-only access to feature gates, limit tests to readonly access Nov 21, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 21, 2018

/milestone v1.14

@k8s-ci-robot k8s-ci-robot added this to the v1.14 milestone Nov 21, 2018

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Nov 21, 2018

A masterpiece of defensive programming. 10/10 would use.

@liggitt liggitt force-pushed the liggitt:verify-unit-test-feature-gates branch from 5196876 to 2498ca7 Nov 21, 2018

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 22, 2018

/retest

1 similar comment
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Nov 26, 2018

/retest

@jennybuckley

This comment has been minimized.

Copy link
Contributor

jennybuckley commented Nov 26, 2018

/cc

@k8s-ci-robot k8s-ci-robot requested a review from jennybuckley Nov 26, 2018

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Nov 29, 2018

/approve
/lgtm

gates ftw

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 29, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 29, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton

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 merged commit 79e5cb2 into kubernetes:master Nov 30, 2018

18 checks passed

cla/linuxfoundation liggitt 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-100-performance 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-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@liggitt liggitt deleted the liggitt:verify-unit-test-feature-gates branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment