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

Features/propagate goflags env #193

Merged
merged 8 commits into from
Jan 28, 2024
Merged

Features/propagate goflags env #193

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4a48392
Propagate GOFLAGS from OS environment.
May 4, 2023
6e1f6a1
Correctly escape just the value when necessary
May 4, 2023
a4c8f0a
Update internal/command/context.go
Bluebugs May 6, 2023
8adafbf
Add unit test.
Jan 6, 2024
80fe656
Try to make the comment less confusing.
Jan 6, 2024
5b88091
Merge branch 'develop' into features/propagate-goflags-env
Jan 6, 2024
4eaf13a
Make sure that we actually return all expected argument.
Jan 11, 2024
fb831a1
Use one uniq `i` for both loop.
Jan 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions internal/command/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"bytes"
"errors"
"fmt"
"os"
"path/filepath"
"runtime"
"strconv"
Expand Down Expand Up @@ -174,6 +175,10 @@ func makeDefaultContext(flags *CommonFlags, args []string) (Context, error) {
return ctx, err
}

if env := os.Getenv("GOFLAGS"); env != "" {
ctx.Env["GOFLAGS"] = env
}

if len(flags.Ldflags) > 0 {
goflags := ""
for _, ldflags := range strings.Fields(flags.Ldflags) {
Expand Down
4 changes: 2 additions & 2 deletions internal/command/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,9 +77,9 @@ func AppendEnv(args []string, environs map[string]string, quoteNeeded bool) []st
for k, v := range environs {
env := k + "=" + v
if quoteNeeded && strings.Contains(v, "=") {
// engine requires to double quote the env var when value contains
// engine requires to double quote the value when it contains
// the `=` char
env = fmt.Sprintf("%q", env)
env = fmt.Sprintf("%s=%q", k, v)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this could have a unit test? It potentially could have a big impact on other variable appending and we have seen a few bugs recently that suggest that not all envs encode the same...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not to sure what the test could be that doesn't look like just enforcing the same code as here, since it is following docker flags interface. Maybe we should exec docker and podman in our tests for that?

Copy link
Member

Choose a reason for hiding this comment

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

It could be factored out to a helper that formats the arts and demonstrates that non-trivial inputs (spaces etc) create the right format of string.
Just a little sanity checking really as a simple "x=y" seems like a potential spot for glitches in env and cmd handling.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Such a tests would enforce and lock implementation details, that are not necessarily correct. That's where I have trouble imagining a test without having to spin a docker and podman command line that is not locking implementation, but actually enforce that it work.

Copy link
Member

Choose a reason for hiding this comment

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

I am totally lost - why does passing an env into a printf require a docker container?

Copy link
Member

Choose a reason for hiding this comment

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

If that was the case then the comment "double quote the env var when value contains the = char" would surely have read "double quote the env value when value contains the = char", as the environment variable is in the KEY=VALUE format is it not?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with Andrew. It is better to verify that the code does what we think is right than not testing it at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If that was the case then the comment "double quote the env var when value contains the = char" would surely have read "double quote the env value when value contains the = char", as the environment variable is in the KEY=VALUE format is it not?

I see and understand that it might be confusing. In that case, any suggestion that would be less confusing for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is better to verify that the code does what we think is right than not testing it at all.

I disagree on this statement. Tech debt for tests is worse than no tests. Never the less, it seems their is a consensus for having this kind of tests in, so I will oblige and add one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test and comment change are in.

}
args = append(args, "-e", env)
}
Expand Down
103 changes: 103 additions & 0 deletions internal/command/docker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,109 @@ func TestCmdEnginePodman(t *testing.T) {
}
}

func TestAppendEnv(t *testing.T) {
type args struct {
args []string
env map[string]string
quoteNeeded bool
}
tests := []struct {
name string
args args
wantStart []string
wantEnd [][2]string
}{
{
name: "empty",
args: args{
args: []string{},
env: map[string]string{},
quoteNeeded: true,
},
wantStart: []string{},
wantEnd: [][2]string{},
},
{
name: "quote needed",
args: args{
args: []string{},
env: map[string]string{"VAR": "value"},
quoteNeeded: true,
},
wantStart: []string{},
wantEnd: [][2]string{{"-e", "VAR=value"}},
},
{
name: "quote not needed",
args: args{
args: []string{},
env: map[string]string{"VAR": "value"},
quoteNeeded: false,
},
wantStart: []string{},
wantEnd: [][2]string{{"-e", "VAR=value"}},
},
{
name: "multiple",
args: args{
args: []string{},
env: map[string]string{"VAR": "value", "VAR2": "value2"},
quoteNeeded: true,
},
wantStart: []string{},
wantEnd: [][2]string{{"-e", "VAR=value"}, {"-e", "VAR2=value2"}},
},
{
name: "multiple with args",
args: args{
args: []string{"arg1", "arg2"},
env: map[string]string{"VAR": "value", "VAR2": "value2"},
quoteNeeded: true,
},
wantStart: []string{"arg1", "arg2"},
wantEnd: [][2]string{{"-e", "VAR=value"}, {"-e", "VAR2=value2"}},
},
{
name: "multiple with args and equal sign require quoting values",
args: args{
args: []string{"arg1", "arg2"},
env: map[string]string{"VAR": "value", "VAR2": "value2=2"},
quoteNeeded: true,
},
wantStart: []string{"arg1", "arg2"},
wantEnd: [][2]string{{"-e", "VAR=value"}, {"-e", "VAR2=\"value2=2\""}},
},
{
name: "multiple with args and equal sign do not require quoting values",
args: args{
args: []string{"arg1", "arg2"},
env: map[string]string{"VAR": "value", "VAR2": "value2=2"},
quoteNeeded: false,
},
wantStart: []string{"arg1", "arg2"},
wantEnd: [][2]string{{"-e", "VAR=value"}, {"-e", "VAR2=value2=2"}},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := AppendEnv(tt.args.args, tt.args.env, tt.args.quoteNeeded)
for i, v := range tt.wantStart {
assert.Equal(t, v, got[i])
}
for i := len(tt.wantStart); i < len(got); i += 2 {
Copy link
Member

Choose a reason for hiding this comment

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

isn't len(tt.wantStart) always going to be the same as i?
It took a while to realise this was contiguous, so I would suggest that I spans the two for loops so that is clear.

I also wondered, wouldn't it make sense to simplfy to just have all the output in a single want that is iterated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue is that you can't rely on hash map order, which is why there is two loops. One for the part that should be stable and one for the part that can vary.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. But that wouldn't impact the usage of i - a single variable would better indicate that it is checking every item in the slices, even though split over two loops?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what you exactly mean. I reused the same name (i), but did want to use the cleaner, more readable, call to range when possible.

Copy link
Member

Choose a reason for hiding this comment

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

I mean that i := len(tt.wantStart) creates a new i with the same value as the old one.
But for me at least this was not obvious... so it seemed sensible that this could be skipped and re-use the same i for both loops? You can still use the range as you have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This actually make the code a bit more complex to read. range does exit the loop with i == len(tt.wantStart) - 1, but if there is nothing in tt.wantStart, it exit with i == 0. So before I can use it I have to add a test to see if len(tt.wantStart) != 0, to add 1 back. This is a lot less readable at the end that with the current code I think. Do you have any suggestion on how to solve that? I can only imagine with going with the C style loop syntax instead of range.

Copy link
Member

Choose a reason for hiding this comment

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

I think you could increment i inside the range loop too, if that's better?

Copy link
Contributor Author

@Bluebugs Bluebugs Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean that i would not be managed by range, but manually? Will have to try that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a big fan, but if that's what you had in mind, I am fine with it.

Copy link
Member

Choose a reason for hiding this comment

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

The notation may not be totally awesome, but it helps to show we are actually only iterating once, despite two loops.
Thanks for the update.

found := false
for _, v := range tt.wantEnd {
if v[0] == got[i] && v[1] == got[i+1] {
found = true
break
}
}
assert.Equal(t, true, found)
}
})
}
}

func TestMain(m *testing.M) {
os.Unsetenv("SSH_AUTH_SOCK")
os.Exit(m.Run())
Expand Down
Loading