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

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

Merged
merged 6 commits into from
Nov 30, 2018
Merged

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

merged 6 commits into from
Nov 30, 2018

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Nov 21, 2018 •
edited

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 ensure feature gate changes don't escape unit tests #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`.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
@k8s-ci-robot k8s-ci-robot added area/apiserver area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 21, 2018
@liggitt liggitt force-pushed the verify-unit-test-feature-gates branch from 3a91aa0 to 6228226 Compare November 21, 2018 15:21
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 21, 2018
@liggitt liggitt changed the title WIP - Verify unit test feature gates Verify unit test feature gates Nov 21, 2018
@liggitt liggitt force-pushed the verify-unit-test-feature-gates branch from 6228226 to 15756bc Compare November 21, 2018 15:46
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. sig/storage Categorizes an issue or PR as relevant to SIG Storage. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 21, 2018
@liggitt liggitt mentioned this pull request Nov 21, 2018
Closed
@k8s-ci-robot k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 21, 2018
@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
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
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 21, 2018
@smarterclayton
Copy link
Contributor

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

@liggitt liggitt force-pushed the verify-unit-test-feature-gates branch from 5196876 to 2498ca7 Compare November 21, 2018 16:52
@liggitt
Copy link
Member Author

liggitt commented Nov 22, 2018

/retest

1 similar comment
@liggitt
Copy link
Member Author

liggitt commented Nov 26, 2018

/retest

@jennybuckley
Copy link

/cc

@smarterclayton
Copy link
Contributor

/approve
/lgtm

gates ftw

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

[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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 29, 2018
@k8s-ci-robot k8s-ci-robot merged commit 79e5cb2 into kubernetes:master Nov 30, 2018
@liggitt liggitt deleted the verify-unit-test-feature-gates branch December 3, 2018 14:34
pohly added a commit to pohly/external-provisioner that referenced this pull request Apr 2, 2019
@pohly
vendor: update to k8s.io 1.14, avoid glog
0334a68 
The previous vendor update ended up pulling in a release-1.10 branch
of k8s.io/apiserver and with it glog, which broke the resulting
executable (compiled, but failed during init due the double definition
of glog flags).

We have to ensure that this does not happen by explicitly choosing
k8s.io package revisions. In this case, 1.14.0 is used (latest
stable), which has a slight API change (see
kubernetes/kubernetes#71302).
@pohly pohly mentioned this pull request Apr 2, 2019
Merged
@liggitt liggitt added the kind/bug Categorizes issue or PR as related to a bug. label Feb 21, 2020
@liggitt liggitt mentioned this pull request Mar 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsalamat bsalamat Awaiting requested review from bsalamat

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@jennybuckley jennybuckley Awaiting requested review from jennybuckley

Assignees

@smarterclayton smarterclayton

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.14
4 participants
@liggitt @smarterclayton @jennybuckley @k8s-ci-robot