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

Add process_env provider #129

Merged
merged 9 commits into from
Sep 4, 2022
Merged

Add process_env provider #129

merged 9 commits into from
Sep 4, 2022

Conversation

kevbook
Copy link
Contributor

@kevbook kevbook commented Jul 30, 2022

Related Issues

#116

Description

Add process_env provider. Load the environment variables from the parent process as needed.

Example Config

providers:
  process_env:
    env_sync:

    env:
      ETC_DSN:

Checklist

  • Tests
  • Documentation
  • Linting

Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

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

@kevbook overall looks good! thank you.

I have few main points

  1. Move env_sync which get all the env var to disable by default and (less secure) and explain how to enable it.
  2. When using env section, the key should be a teller key and the value should be what teller need to pick.
  3. Please add test in process_env_test.go

README.md Outdated
providers:
process_env:
env_sync:

Copy link
Contributor

Choose a reason for hiding this comment

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

How can I disable to use env_sync which bring me all the environment variable? Should i comment env_sync section?

If yes, I think this should be the default in pkg/wizard_template.go with a comment description that this way that, bring all environment variables is less secure, and it is better to pick only what you need.

WDYT?

Copy link
Contributor Author

@kevbook kevbook Aug 5, 2022

Choose a reason for hiding this comment

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

This makes sense.

How do you feel about not having an env_sync: option? Firstly, it's same as carry_env, and secondly, that's not the intention of the provider really. @kaplanelad

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can keep it for some usecases. but in default we can commented it out or totally remove it from the default config.
Sound good?

pkg/providers/process_env.go Outdated Show resolved Hide resolved

// DeleteMapping will delete the given path recessively
func (a *ProcessEnv) DeleteMapping(kp core.KeyPath) error {
return nil
Copy link
Contributor

@kaplanelad kaplanelad Aug 3, 2022

Choose a reason for hiding this comment

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

Suggested change
return nil
return fmt.Errorf("%s does not implement delete mapping yet", a.Name())

Copy link
Contributor

@kaplanelad kaplanelad Aug 9, 2022

Choose a reason for hiding this comment

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

please changed the delete to deleteMapping

pkg/providers/process_env.go Outdated Show resolved Hide resolved

// PutMapping will create a multiple entries
func (a *ProcessEnv) PutMapping(p core.KeyPath, m map[string]string) error {
return nil
Copy link
Contributor

@kaplanelad kaplanelad Aug 3, 2022

Choose a reason for hiding this comment

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

Suggested change
return nil
return fmt.Errorf("%s does not implement putMapping yet", a.Name())

{{- if index .ProviderKeys "process_env" }}

process_env:
env_sync:
Copy link
Contributor

@kaplanelad kaplanelad Aug 3, 2022

Choose a reason for hiding this comment

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

See my comment in README.md file.

env_sync:

env:
ETC_DSN:
Copy link
Contributor

Choose a reason for hiding this comment

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

From my understanding, this setup get the value ETC_DSN from env var
for example

$ export ETC_DSN=teller-value
$ teller yaml
ETC_DSN: teller-value

It will be better if we also allow the user to control the key name.
for example
the key under env should be the teller key, and the value is the name of the env key that we want to pick up.
.teller config:
.teller config:

providers:

  process_env:
    env:
       TELLER-KEY: ETC_DSN

Execute teller:

$ export ETC_DSN=teller-value
$ teller yaml
TELLER-KEY:: teller-value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it'll use field to be consistent with other providers. So:

process_env:
  env:
    GH_USERNAME: # will use this field
    GH_TOKEN:
      field: TOKEN # will use this field to map to GH_TOKEN

@kaplanelad kaplanelad added the new-provider A new provider suggestion label Aug 3, 2022
@kaplanelad
Copy link
Contributor

Hey @kevbook,
Just summarize the open issues in this PR:

  1. remove env_sync from the config example meaning by default the env_sync will not generate when running teller new. but pkg/providers/process_env.go keep the GetMapping function to support env_sync in some usecases
  2. Return errors (not nil) in places that we not implement the logic (see my change in suggestions), you also need to import fmt see your CI results
  3. Add process_env_test.go

thanks for your help

@kevbook
Copy link
Contributor Author

kevbook commented Aug 30, 2022

I've updated the code, wizard_template and the README with usage.

I don't really write go, so I am not sure how tests can be written. But I can take a stab at process_env_test.go if you'd like to review the code.

Copy link
Contributor

@kaplanelad kaplanelad left a comment

Choose a reason for hiding this comment

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

Thanks @kevbook for this PR

@kaplanelad kaplanelad merged commit 97ef49b into tellerops:master Sep 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-provider A new provider suggestion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants