Skip to content
This repository was archived by the owner on Apr 4, 2023. It is now read-only.

Add feature flag support #339

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add feature flag support #339

wants to merge 2 commits into from

Conversation

kragniz
Copy link
Contributor

@kragniz kragniz commented Apr 24, 2018

What this PR does / why we need it: adds support for generic feature flags

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #335

Special notes for your reviewer:

Release note:

Add support for feature flags

@jetstack-bot jetstack-bot requested a review from wallrj April 24, 2018 16:56
@jetstack-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: munnerz

Assign the PR to them by writing /assign @munnerz in a comment when ready.

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

kragniz
kragniz commented Apr 24, 2018
View reviewed changes
pkg/util/feature/feature_gate.go
limitations under the License.
*/

// NOTE: this file is based off k8s.io/apiserver/pkg/util/feature
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was originally making some changes here, but they ended up not being necessary. Can we just pull this straight from k8s.io/apiserver/pkg/util/feature?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, and it looks like it's already in vendor/.

wallrj
wallrj approved these changes Apr 26, 2018
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks @kragniz.

Looks like a good start.

  • Add a note about feature flags to the developer docs and maybe the user docs.
  • Enable the example feature flag in the E2E tests
  • Import rather than copy code from k8s.io/apiserver as you suggested.
  • Maybe quickly summarise what you were telling me about how kubeadm put feature flags in the API resources....so that I can enable features on a per-cluster basis...might we do that as well....or would that be equivalent to making an v2alpha1 API?

contrib/charts/navigator/values.yaml
@@ -6,6 +6,9 @@ createAPIService: true
rbac:
enabled: true

# A set of key=value pairs that describe feature gates for various features. Use --help to see available flags.
featureGates: ""
Copy link
Member

Choose a reason for hiding this comment

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

Comma or space separated?

cmd/controller/app/options/options.go
@@ -66,6 +76,8 @@ func (s *ControllerOptions) AddFlags(fs *pflag.FlagSet) {
fs.DurationVar(&s.LeaderElectionRetryPeriod, "leader-election-retry-period", defaultLeaderElectionRetryPeriod, ""+
"The duration the clients should wait between attempting acquisition and renewal "+
"of a leadership. This is only applicable if leader election is enabled.")
fs.StringVar(&s.FeatureGatesString, "feature-gates", defaultFeatureGatesString, "A set of key=value pairs that describe feature gates for various features. "+
"Options are:\n"+strings.Join(features.KnownFeatures(&features.InitFeatureGates), "\n"))
Copy link
Member

Choose a reason for hiding this comment

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

Comma or space separated?

@wallrj
Copy link
Member

wallrj commented Apr 26, 2018

The output looks like this:

./navigator-controller_linux_amd64 --help

cert-manager is a Kubernetes addon to automate the management and issuance of
TLS certificates from various issuing sources.

It will ensure certificates are valid and up to date periodically, and attempt
to renew certificates at an appropriate time before expiry.

Usage:
  cert-manager-controller [flags]

Flags:
      --alsologtostderr                           log to standard error as well as files
      --feature-gates string                      A set of key=value pairs that describe feature gates for various features. Options are:
Example=true|false (ALPHA - default=false)
...

@jetstack-bot
Copy link
Collaborator

@kragniz: PR needs rebase.

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.

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

@wallrj wallrj wallrj approved these changes

Assignees
No one assigned
Labels
needs-rebase release-note size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature flag support
3 participants
@kragniz @jetstack-bot @wallrj