feat: allow substitutions in plugin env variables #6097
Conversation
Signed-off-by: Leah <github.leah@hrmny.sh>
Signed-off-by: Leah <github.leah@hrmny.sh>
Codecov Report
@@ Coverage Diff @@
## master #6097 +/- ##
==========================================
+ Coverage 40.93% 41.03% +0.09%
==========================================
Files 147 151 +4
Lines 19729 19960 +231
==========================================
+ Hits 8077 8191 +114
- Misses 10541 10640 +99
- Partials 1111 1129 +18
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks a lot for your contribution, @ForsakenHarmony!
I have some comments, see below.
reposerver/repository/repository.go
Outdated
| for i, v := range env { | ||
| parsedVar, err := v1alpha1.NewEnvEntry(v) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Can you please create a custom error here using fmt.Errorf() instead of passing down err - because it might contain the value of the erroneous environment variable, which ends up in the logs eventually.
There was a problem hiding this comment.
What do you think we should return here? It would be nice to know the name of the env var that failed, but I suppose that's not really possible without potentially compromising a secret
I'm also wondering if it's even possible for this to happen
There was a problem hiding this comment.
I think the error might contain env variable defined in ApplicationSource only, not the local env variables of the repo-server pod.
The ApplicationSource is not considered secret and already appears in repo-server logs, so I'm fine to keep it as is.
| @@ -33,7 +33,7 @@ Commands have access to | |||
|
|
|||
| 1. The system environment variables | |||
| 2. [Standard build environment](build-environment.md) | |||
| 3. Variables in the application spec: | |||
| 3. Variables in the application spec (you can substitute any of the variables from the last 2 points): | |||
There was a problem hiding this comment.
Nitpicking: I think it is hard to understand the meaning of this sentence, can we rephrase it a little? E.g. "References to system and build variables will get interpolated in the variables' values."
There was a problem hiding this comment.
Yes that sounds good, I wasn't happy with it either
Tried coming up with a nice way to phrase it 3 times, but that didn't work out, I'm adding your suggestion
Signed-off-by: Leah <github.leah@hrmny.sh>
Signed-off-by: Leah <github.leah@hrmny.sh>
There was a problem hiding this comment.
Thank you for addressing the review concerns, @ForsakenHarmony
LGTM now!
Related to #5293
Until there's a more robust solution for custom plugins I wanted to be able to pass specific args including e.g. the revision to
tankaChecklist: