-
Notifications
You must be signed in to change notification settings - Fork 385
Deprecate storage-type flag #2266
Deprecate storage-type flag #2266
Conversation
/assign @carolynvs |
why do we want to show the flag in help? to act as notification that it is deprecated? |
Correct. This is as following Kubernetes conventions - see https://github.com/kubernetes/kubernetes/blob/86b9a53226b1c9f9dce3ffb0133482f14709418b/cmd/controller-manager/app/options/insecure_serving.go#L69-L71 for an example. |
I've refactored this following spf13/pflag#163 to use the "proper" |
) | ||
|
||
const ( | ||
// Store generated SSL certificates in a place that won't collide with the | ||
// k8s core API server. | ||
certDirectory = "/var/run/kubernetes-service-catalog" | ||
|
||
storageTypeFlagName = "storageType" | ||
storageTypeFlagName = "storage-type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't change the flag, as far as I can tell. We just weren't using it before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wasn't being used before, but it is now - see https://github.com/kubernetes-incubator/service-catalog/pull/2266/files#diff-5dd3f709c02872b9414e257945813685R89 and following lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
big cleanup.
@carolynvs Any chance of a review of this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
"The type of backing storage this API server should use", | ||
) | ||
flags.MarkDeprecated(storageTypeFlagName, "The value of this flag is now unused and will be removed in the near future") | ||
flags.Lookup(storageTypeFlagName).Hidden = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carolynvs 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 |
@carolynvs Thanks for the review! Is there anyone else that can review so we could get this merged? Thanks! |
/assign @jboyd01 Jay, I'm hoping you have time to give this a lgtm? 😀 |
sorry this has taken so long to get merged @jimmidyson, we appreciate the contribution! |
This uses the same conventions as Kubernetes, not marking flag as deprecated via pflag.FlagSet because this hides the flag from help, but rather marking the usage message as DEPRECATED.
As discussed in #2201, this keeps the flag and chart values to not break existing deployments, but completely unwires it so it can be removed in a (near-)future release.