Features/propagate goflags env #193

Bluebugs · 2023-05-04T21:50:57Z

Description:

This PR will pick GOFLAGS from fyne-cross host environment and propagate them to the call in the container.

Where applicable:

Public APIs match existing style.

Jacalz

Nice. Just a little comment in-line about not calling os.Getenv() twice to avoid needing two system calls if the environment variable is set up.

internal/command/context.go

andydotxyz · 2023-05-06T09:27:09Z

internal/command/docker.go

@@ -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)


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...

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?

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.

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.

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

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 agree with Andrew. It is better to verify that the code does what we think is right than not testing it at all.

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?

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.

Test and comment change are in.

Co-authored-by: Jacob Alzén <jacob.alzen@gmail.com>

mikaelnousiainen · 2024-01-06T08:50:48Z

Would be great to have this merged and released asap now that the tests are in!

andydotxyz

Personally I would have factored out the escaping code to a smaller scope so that could be tested with much simpler unit code.
However this certainly covers the usage - but as commented inline isn't it easier / clearer just to iterate over one want for the args list?

andydotxyz · 2024-01-07T17:34:54Z

internal/command/docker_test.go

+			for i, v := range tt.wantStart {
+				assert.Equal(t, v, got[i])
+			}
+			for i := len(tt.wantStart); i < len(got); i += 2 {


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?

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.

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?

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.

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.

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.

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

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

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

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.

andydotxyz

Thanks for this

Cedric BAIL added 2 commits May 4, 2023 15:49

Propagate GOFLAGS from OS environment.

4a48392

Correctly escape just the value when necessary

6e1f6a1

Bluebugs mentioned this pull request May 5, 2023

fyne-cross option -ldflags does not work properly like it does in go build #190

Closed

Jacalz requested changes May 6, 2023

View reviewed changes

internal/command/context.go Outdated Show resolved Hide resolved

andydotxyz reviewed May 6, 2023

View reviewed changes

Update internal/command/context.go

a4c8f0a

Co-authored-by: Jacob Alzén <jacob.alzen@gmail.com>

Bluebugs requested a review from Jacalz December 3, 2023 21:11

Jacalz previously approved these changes Dec 4, 2023

View reviewed changes

Cedric BAIL added 2 commits January 5, 2024 21:25

Add unit test.

8adafbf

Try to make the comment less confusing.

80fe656

Bluebugs dismissed Jacalz’s stale review via 80fe656 January 6, 2024 04:26

Merge branch 'develop' into features/propagate-goflags-env

5b88091

andydotxyz reviewed Jan 7, 2024

View reviewed changes

ofman approved these changes Jan 9, 2024

View reviewed changes

Make sure that we actually return all expected argument.

4eaf13a

Bluebugs requested a review from andydotxyz January 25, 2024 05:12

Use one uniq i for both loop.

fb831a1

andydotxyz approved these changes Jan 28, 2024

View reviewed changes

Bluebugs merged commit 5448401 into develop Jan 28, 2024
56 checks passed

Bluebugs deleted the features/propagate-goflags-env branch January 28, 2024 21:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/propagate goflags env #193

Features/propagate goflags env #193

Bluebugs commented May 4, 2023

Jacalz left a comment

andydotxyz May 6, 2023

Bluebugs Dec 3, 2023

andydotxyz Dec 4, 2023

Bluebugs Dec 5, 2023

andydotxyz Dec 6, 2023

andydotxyz Jan 5, 2024

Jacalz Jan 5, 2024

Bluebugs Jan 6, 2024

Bluebugs Jan 6, 2024

Bluebugs Jan 6, 2024

mikaelnousiainen commented Jan 6, 2024

andydotxyz left a comment

andydotxyz Jan 7, 2024

Bluebugs Jan 7, 2024

andydotxyz Jan 9, 2024

Bluebugs Jan 11, 2024

andydotxyz Jan 12, 2024

Bluebugs Jan 14, 2024

andydotxyz Jan 27, 2024

Bluebugs Jan 28, 2024 •

edited

Bluebugs Jan 28, 2024

andydotxyz Jan 28, 2024

andydotxyz left a comment

Features/propagate goflags env #193

Features/propagate goflags env #193

Conversation

Bluebugs commented May 4, 2023

Description:

Where applicable:

Jacalz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikaelnousiainen commented Jan 6, 2024

andydotxyz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bluebugs Jan 28, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andydotxyz left a comment

Choose a reason for hiding this comment

Bluebugs Jan 28, 2024 •

edited