WIP: Add feature status to environment vars #266
Conversation
istio-testing
added
the
do-not-merge/work-in-progress
|
😊 Welcome @therealmitchconnors! This is either your first contribution to the Istio pkg repo, or it's been You can learn more about the Istio working groups, code of conduct, and contributing guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
google-cla
bot
added
the
cla: yes
istio-testing
added
the
size/L
| case Beta: | ||
| return "Beta" | ||
| case Stable: | ||
| return "Stable" |
There was a problem hiding this comment.
What does it mean for an env var to be stable? Don't we currently document that all env vars are alpha?
Put another way, why would we promote an env var to stable vs moving it to an API/mesh config?
There was a problem hiding this comment.
I am all for the idea of migrating all stable environment variables to an API, but that does not represent the current state of the project. For instance, I believe we set the PILOT_TRACE_SAMPLING variable on every istio deployment, and it should probably be considered stable.
There was a problem hiding this comment.
PILOT_TRACE_SAMPLING will be deprecated in 1.9 by the tracing api. We shouldn't mark something "stable" just because its been around for a while, we should mark it stable if we know its the right long term API and its properly tested
There was a problem hiding this comment.
The overall idea here is to give users a way to visualize the maturity of the features they are leveraging. We've already got that for Annotations with Nate's PR, and this would provide visibility into env vars, which could be combined in a single istioctl command to show users what features they are using, and what support they can expect. Given the prominence of many env vars, I doubt we would feed comfortable telling our users that all of them have only alpha level support.
There was a problem hiding this comment.
Can we track the same state we're now tracking for annotation to cover the transition intent
deprecated, deprecatedBy
etc.
There was a problem hiding this comment.
Yes. It might be helpful to track the removal of deprecated features too, so we can highlight that when checking upgrade readiness.
It's also useful to note that users may not be setting these directly, but via operator or helm values.
There was a problem hiding this comment.
Should we add experimental to here as well?
| type FeatureStatus byte | ||
|
|
||
| const ( | ||
| Alpha FeatureStatus = iota |
There was a problem hiding this comment.
In a world where we canonicalize these Unknown's should become forbidden. Can we add a 'Dev' status
I think there's also a class of 'industry std' environment variables that we depend on which would not belong to our canonicalization. (PATH, USER, ...) How would we handle those? Maybe add a "Platform" ?
There was a problem hiding this comment.
Platform env vars are not registered with the env package, and the ctrlz page lists their default values as UNKNOWN. I don't think we have any concept of 'Dev' status elsewhere in the project. Would 'experimental' make more sense?
|
What format does the ctrlz page take? Hopefully the same one we will use for the checked-in canonicalization. |
You can see the format here: https://drive.google.com/file/d/1EU9f711kP4TR6ixttN7GFohVcw48AGwR/view?usp=sharing |
|
In the UI it says the 'Set of environment variables defined for the process" which implies we're dumping everything. Does it make sense to be more selective? Options include
WDYT? |
|
The env vars are already sorted to show known env vars first. I am happy to move the env vars which are not known to the ctrlz system (and therefore have no default) into a separate table if that helps. |
istio-testing
added
the
needs-rebase
|
@therealmitchconnors: 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. |
|
What do we want the ctrlz page to show by default?
- All possible env vars
- All 'set' env vars
- All 'set' env vars with a non-default value
I suspect the latter would be the most useful. Variety of presentations
possible to achieve delineation (sorting, split tables etc.)
…On Sat, Jan 9, 2021 at 10:37 PM Istio Automation ***@***.***> wrote:
@therealmitchconnors <https://github.com/therealmitchconnors>: PR needs
rebase.
Instructions for interacting with me using PR comments are available here
<https://git.k8s.io/community/contributors/guide/pull-requests.md>. If
you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#266 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACFAUPHDD234YJ74Z4Q3XQDSZFDI5ANCNFSM4TKSEQ2A>
.
|
This adds the concept of feature support for our various environment variables, and sets all variables to unknown for now. Users can call SetVarStatus to update the support level of a given env var.