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 3 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 @@ -173,6 +174,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
2 changes: 1 addition & 1 deletion internal/command/docker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func AppendEnv(args []string, environs map[string]string, quoteNeeded bool) []st
if quoteNeeded && strings.Contains(v, "=") {
// engine requires to double quote the env var when value 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
Loading