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.

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

Enable users to specify default flag values for the apps plugin via environment variables #114

Closed
heyjcollins opened this issue May 11, 2022 · 9 comments · Fixed by #269
Closed
Assignees
Labels
accepted enhancement New feature or request
Milestone

Comments

@heyjcollins
Copy link
Contributor

heyjcollins commented May 11, 2022

Description of problem

Developers will provide the same flags/values repeatedly when iterating on their application code.
Typing or copy/pasting the flag values for every workload create/update/apply adds friction to the developer workflow.
We should make it possible for the user to specify sane defaults via envvars such that they can execute their commands with the minimum required flags


Related to #108


Proposed solution

  • All supported EnvVars are documented in all discoverable doc locations
  • Envvar naming convention is established, documented and discoverable
  • When an envvar exists for a given flag and the flag is not explicitly provided, use the envvar value
  • When an envar exists and a flag/value HAS been explicitly provided, use the flag value
  • Where no env var or flag/value exists for a required flag, throw an error

Precedence order: flag/value > environment variable > config value [*1]
[1] if/when we work on #108 (enabling users to configure persistent flag values...) --- the EnvVar should take precedence over the configured value for a given flag

Command output changes:
Output from tanzu apps workload create/update/apply should remain as-is (no changes are required to support this new feature).

naming convention for the env vars:

  • --flag-name ==> TANZU_APPS_[FLAG_NAME]
FLAG ENV VAR
--debug TANZU_APPS_DEBUG
--file TANZU_APPS_FILE
--git-repo TANZU_APPS_GIT_REPO
--live-update TANZU_APPS_LIVE_UPDATE
--local-path TANZU_APPS_LOCAL_PATH
--namespace TANZU_APPS_NAMESPACE
--registry-ca-cert TANZU_APPS_REGISTRY_CA_CERT
--registry-password TANZU_APPS_REGISTRY_PASSWORD
--registry-username TANZU_APPS_REGISTRY_USERNAME
--registry-token TANZU_APPS_REGISTRY_TOKEN
--source-image TANZU_APPS_SOURCE_IMAGE
--sub-path TANZU_APPS_SUB_PATH
--type TANZU_APPS_TYPE
*NOTE: strikethrough items have been removed from initial scope for this feature. We'll re-evaluate post feature release to determine what additional envvar/flags would be valuable to support.

Describe alternatives you've considered

use only configurable values

@scothis
Copy link
Contributor

scothis commented May 11, 2022

@heyjcollins is there prior art for this approach that you think is particularly well done?

  • Environment variables are typically used to define context for the execution of a command, and less to provide specific inputs.
  • A prefix should be added to each env to make it unambiguously tied to this command. A var like IMAGE or ENV could quite easily exist in the environment but not be what the user intended.
  • An info line should be logged when a value is read from the environment, otherwise it could lead to significant user confusion as to why a value is being set that they didn't ask for.
  • For flags that can be set multiple type, how are they distinguished from an env key that can only be defined once?
  • Viper is commonly used with cobra (same author) to munge config from multiple sources.

@heyjcollins
Copy link
Contributor Author

Thanks for the feedback and suggestions @scothis.

Originally, the plan was to provide a new (for apps plugin) config command solution to address the same issue (#108) - and this was in-addition-to the envvar based solution described here).

Upon further consideration, we've decided to pursue only the config solution.

Why

  • multiple solutions adds complexity without much additional value
  • multiple solutions may unnecessarily increase confusion/problems for users if/when they experience unexpected outcomes and have to sleuth through the env vars AND configs.
  • the config command solution can/will be more easily discoverable while using the plugin
  • there's no chance of collisions since the config will be specific to the apps plugin

So...For the time being, and until/unless we get user feedback indicating the need for the alternative env var solution, we're going to implement (#108).

Your suggestions/questions here contributed to this decision and will inform our work on #108 so thank you very much.

@heyjcollins
Copy link
Contributor Author

closing this in favor of #108.
we can reopen if the need arises post #108.

@heyjcollins
Copy link
Contributor Author

Because we haven't established common pattern for persisting configuration across all plugins and tanzu CLI versions, we've decided to put #108 on hold 'till we've aligned on an approach.
In the meantime, I'm reopening this issue for execution.

@heyjcollins heyjcollins reopened this Aug 8, 2022
@heyjcollins heyjcollins added this to the 0.9.0 milestone Aug 8, 2022
@rashedkvm
Copy link
Member

rashedkvm commented Aug 15, 2022

Values from environment variables were applied:
TANZU_APPS_SOURCE_IMAGE:
TANZU_APPS_SOURCE_REPO:

@heyjcollins Values from env var can contain sensitive data and should not output as plain text. Workload diff seems sufficient to me and would vote to drop this section from the output.

@heyjcollins
Copy link
Contributor Author

heyjcollins commented Aug 16, 2022

This is a good point.
if we show it in the diff, then it's still being exposed in the terminal.
I'd like to consider this (showing sensitive values as <redacted> in the output) more broadly with @danfein

@heyjcollins
Copy link
Contributor Author

Decisions:

  1. Initial scope of supported envvars will be limited to 6 flags (source-image, registry*, and type)
  2. the envvar/value pairs will NOT be displayed in command output and we'll rely on documentation for discovery (in a subsequent release we'll provide a new command tanzu apps env to show which env's are supported, which are set, and if desired, what the values are) --- issue forthcoming.

@scothis
Copy link
Contributor

scothis commented Aug 16, 2022

Any value written to a K8s resource (that is not a Secret) is inherently not sensitive.

Values from TANZU_APPS_REGISTRY_PASSWORD or TANZU_APPS_REGISTRY_TOKEN are not written to the Workload and are sensitive.

@heyjcollins
Copy link
Contributor Author

There are some nuanced scenarios whereby the TANZU_APPS_SOURCE_IMAGE envvar value should or should not be applied when a workload create/apply/update is executed.
These scenarios weren't accounted for in the original solution and they need to be carefully considered.

We've decided to remove the support for TANZU_APPS_SOURCE_IMAGE in the initial release of this feature and take some additional time to consider whether/how to properly support it.

I've updated the original issue description and added strikethrough for the envvar to remove it from scope for the issue.

cc @odinnordico @shashwathi @atmandhol @danfein

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants