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.

Sign up for GitHub

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

Jump to bottom

Clean up helm/config.go #9

Closed
ErinCall opened this issue Dec 17, 2019 · 1 comment · Fixed by #32
Closed

Clean up helm/config.go #9

ErinCall opened this issue Dec 17, 2019 · 1 comment · Fixed by #32
Assignees
@ErinCall
Labels
code quality Improvements to the internals that may not have user-facing impact documentation Improvements or additions to documentation good first issue Good for newcomers

Comments

@ErinCall
Copy link
Contributor

ErinCall commented Dec 17, 2019 •
edited
Loading

The Config struct in internal/helm/config.go defines the interface between drone-helm3 and a project's Drone settings. As such, it should be clear and well-documented:

  • Make sure the comment above the Config struct makes sense to you—it makes sense to me, but I'm a poor judge since I already know all the things it's telling me.
  • Remove the "Global helm config" and "Config specifically for helm upgrade" comments; they aren't particularly accurate
  • Make sure each struct field has a comment explaining what that setting does
  • Probably remove the custom decoder for helmCommand; that responsibility lies with the determineSteps function in internal/helm/plan.go.
  • Fix the inthe typo on line 8 🙃
@ErinCall ErinCall added documentation Improvements or additions to documentation good first issue Good for newcomers labels Dec 17, 2019
@ErinCall
Copy link
Contributor Author

Currently, there's some code that tries to guess which config fields might show up in the environment section of the consumer's .drone.yml, rather than the settings section. Those fields have to have an envconfig tag, so that envconfig will look for the non-prefixed form if it doesn't see a PLUGIN_ version.

It probably makes more sense to allow all fields to come from a non-prefixed env var. It should be a small change to cmd/drone-helm/main.go. After calling envconfig.Process("plugin", &c), call it again with an empty string for the prefix: envconfig.Process("", &c)

Make sure to confirm that

  1. an empty-string prefix looks for ENV_VAR_NAME, not _ENV_VAR_NAME
  2. if the prefixed form is present, but the non-prefixed form is empty, the second call to envconfig.Process won't stomp on the populated value.

@ErinCall ErinCall added the code quality Improvements to the internals that may not have user-facing impact label Dec 17, 2019
@ErinCall ErinCall mentioned this issue Dec 18, 2019
Closed
@ErinCall ErinCall self-assigned this Dec 23, 2019
ErinCall added a commit that referenced this issue Dec 23, 2019
@ErinCall
Document helm.Config's struct fields more clearly [#9]
ad5baea
ErinCall added a commit that referenced this issue Dec 23, 2019
@ErinCall
No typo inthe helm.Config docs [#9]
ae9cb59
ErinCall added a commit that referenced this issue Dec 23, 2019
@ErinCall
Use a plain string for helm.Config.Command [#9]
ef4db92 
I'm leaving the no-op test file in place because my next step is to add
new behavior that will require testing.
@ErinCall ErinCall mentioned this issue Dec 23, 2019
Merged
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Do envconfig-loading in config.go (and test it!) [#9]
c4c136b
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Process non-prefixed forms of all config settings [#9]
e2f53f3 
Trying to guess in advance which part of the config a user will put in
the `settings` section and which they'll put in `environment` is a
fool's errand. Just let everything go in either place.

The ServiceAccount field only had an `envconfig` tag (as opposed to
`split_words`) because that triggered envconfig to look for the non-
prefixed form. Now that we're finding non-prefixed forms of everything,
we can use the clearer/more concise tag.

Note that TestPopulateWithConflictingVariables isn't meant to say
"here's what behavior we *want*" so much as "here's what the behavior
*is*." I don't think one thing is any better than the other, but we
should know which one we're getting.
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Require no-error in config tests [#9]
db87bd0
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Document the Config struct's behavior correctly [#19] [#9]
10e7e7f
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Use a go-idiomatic constructor for helm.Config [#9]
4ba1e69
ErinCall added a commit that referenced this issue Dec 24, 2019
@ErinCall
Log debug output in helm.Config [#9]
08ddf5e 
Redacting KubeToken may not be sufficient, since it's possible that
someone would put secrets in Values or StringValues. Unilaterally
redacting those seems unhelpful, though, since they may be the very
thing the user is trying to debug. I've settled on redacting the obvious
field without trying to promise that all sensitive data will be hidden.
@ErinCall ErinCall mentioned this issue Dec 24, 2019
Closed
ErinCall added a commit that referenced this issue Dec 30, 2019
@ErinCall
The prefix setting is implemented [#19,#9]
22e30fe 
Just something I noticed while resolving a merge conflict. The "write
some docs" and "implement prefix" branches happened concurrently and
didn't get re-coordinated.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ErinCall ErinCall

Labels
code quality Improvements to the internals that may not have user-facing impact documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Clarify and improve internal/helm.Config pelotech/drone-helm3
1 participant
@ErinCall