frontend/dockerfile: some minor refactor and optimizations by thaJeztah · Pull Request #3199 · moby/buildkit

thaJeztah · 2022-10-19T20:43:20Z

See individual commits for details;

frontend/dockerfile: remove isSSHMountsSupported, isSecretMountsSupported

These became obsolete in 906e345, which always enabled them.

frontend/dockerfile: parseBuildStageName(): pre-compile regex

Before/After:
BenchmarkParseBuildStageName-10 180793 5972 ns/op 8355 B/op 126 allocs/op
BenchmarkParseBuildStageName-10 1980735 605.5 ns/op 48 B/op 3 allocs/op

Also renamed a variable that collided with an import.

frontend/dockerfile/parser: remove redundant concat

frontend/dockerfile/shell: use strings.Equalfold

Also fix some grammar issues in the GoDoc, and some nits in the tests.

Before/after:

 BenchmarkToUpper-10          11979394            96.31 ns/op          10 B/op           2 allocs/op
 BenchmarkEqualFold-10        40687216            29.65 ns/op           0 B/op           0 allocs/op

frontend/dockerfile: define types for enums

This is makes it easier to discover what values are acceptable;

Generated docs shows them nicely together.
Intent is clearer (these are all the MountType options).
Intent on fields and arguments is clearer (func(t MountType), struct Foo{Field: MountType}),
but still allows for using / passing a regular string (without casting); validation
is already handled for these, so invalid strings would (should) already be handled.
With these types, both docs at pkg.go.dev and IDEs allow for easier navigating
(click the MountType in the examples above to navigate to the MountType
definition, and the options (consts) below them.

frontend/dockerfile: provide suggestions for mount share mode

similar to df9781b

frontend/dockerfile: move check for cache-sharing

The m.Type == MountTypeSecret has a more specific check for this option,
which would not be hit because this check was before it.

Move the check until after the validation for secret mounts, so that the
specific error can be returned, instead of the generic one.

frontend/dockerfile: parseMount() use strings.Cut(), and some minor cleanup

use strings.Cut() which simplifies handling of key/value pairs
remove some redundant intermediate variables
combined "fileInfoAllowed" if-statement for easier reading

frontend/dockerfile: parseExtraHosts(): use strings.Cut()

frontend/dockerfile: BFlags.Parse(): use strings.Cut()

Also some minor cleanup in the boolean value parsing; using a switch
instead of if/elseif/else.

frontend/dockerfile/instructions/commands_runmount.go

frontend/dockerfile/instructions/commands_runnetwork.go

aaronlehmann · 2022-10-21T16:21:01Z

frontend/dockerfile/instructions/parse.go

-		if ok, _ := regexp.MatchString("^[a-z][a-z0-9-_\\.]*$", stageName); !ok {
-			return "", errors.Errorf("invalid name for build stage: %q, name can't start with a number or contain symbols", args[2])
+		if !validStageName.MatchString(stageName) {
+			return "", errors.Errorf("invalid name for build stage: %q, name can't start with a number or contain symbols", stageName)


nit: this will include the lowercased version in the error message - is that what we want?

Oh! Good catch; I initially made the regex itself case-insensitive (adding (?i)) but I saw that it introduces more allocations (or memory use) than the existing strings.ToLower() so I changed that back, but forgot to put back this value here.

Let me update that.

aaronlehmann · 2022-10-21T16:24:47Z

frontend/dockerfile/shell/lex_test.go

@@ -44,8 +42,7 @@ func TestShellParserMandatoryEnvVars(t *testing.T) {
 	require.Equal(t, "", newWord)


This could be simplified to require.Empty

Ah! Yes, let me update that as well. I'm not a frequent user of testify, so didn't go look for those. My eye landed on the string matching for errors, which felt "incorrect", so I assumed testify had a function for that (it did), which also produced better output on failures, so I updated them 👍

thaJeztah · 2022-10-21T19:57:40Z

@aaronlehmann thanks for reviewing; I updated some of your suggestions, and also updated the commit message for the "types" commit (to outline what I commented above) Let me know what you think (feedback welcome as always 👍 )

thaJeztah · 2022-11-01T15:47:05Z

@tonistiigi @crazy-max ptal

thaJeztah · 2022-12-04T17:31:06Z

Rebased, after #3305 was merged

@AkihiroSuda @crazy-max @tonistiigi PTAL

frontend/dockerfile/instructions/bflag.go

frontend/dockerfile/instructions/commands_runmount.go

…rted These became obsolete in 906e345, which always enabled them. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Before: BenchmarkParseBuildStageName-10 180793 5972 ns/op 8355 B/op 126 allocs/op After: BenchmarkParseBuildStageName-10 1980735 605.5 ns/op 48 B/op 3 allocs/op Also renamed a variable that collided with an import. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also fix some grammar issues in the GoDoc, and some nits in the tests. Before/after: BenchmarkToUpper-10 11979394 96.31 ns/op 10 B/op 2 allocs/op BenchmarkEqualFold-10 40687216 29.65 ns/op 0 B/op 0 allocs/op Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

This is makes it easier to discover what values are acceptable; - Generated docs shows them nicely together. - Intent is clearer (these are all the `MountType` options). - Intent on fields and arguments is clearer (`func(t MountType)`, `struct Foo{Field: MountType}`), but still allows for using / passing a regular string (without casting); validation is already handled for these, so invalid strings would (should) already be handled. - With these types, both docs at pkg.go.dev and IDEs allow for easier navigating (click the `MountType` in the examples above to navigate to the `MountType` definition, and the options (`const`s) below them. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

similar to df9781b Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

The `m.Type == MountTypeSecret` has a more specific check for this option, which would not be hit because this check was before it. Move the check until after the validation for secret mounts, so that the specific error can be returned, instead of the generic one. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

…leanup - use strings.Cut() which simplifies handling of key/value pairs - remove some redundant intermediate variables - combined "fileInfoAllowed" if-statement for easier reading Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Also some minor cleanup in the boolean value parsing; using a switch instead of if/elseif/else. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah

@jedevc updated;

changed types from an alias to a strong type
merged the !ok check

I also pushed 2 additional commits;

frontend/dockerfile: provide suggestions for mount share mode
frontend/dockerfile: move check for cache-sharing

thaJeztah · 2022-12-05T12:28:13Z

frontend/dockerfile/instructions/commands_runmount.go

+			v := ShareMode(strings.ToLower(value))
+			if _, ok := allowedSharingModes[v]; !ok {
+				return nil, suggest.WrapError(errors.Errorf("unsupported sharing value %q", value), value, allShareModes(), true)


I based this on df9781b, but am slightly confused, as the suggest.WrapError() uses caseSensitive=true, but the code here suggests otherwise (perhaps this was meant to be an optimisation to not have to do the strings.ToLower()? But in that case we should pass v to the function, not value.

crazy-max

LGTM, ptal @jedevc @tonistiigi

jedevc · 2023-01-03T15:42:53Z

frontend/dockerfile/instructions/commands_runmount.go

 	}

+	if m.CacheSharing != "" && m.Type != MountTypeCache {
+		return nil, errors.Errorf("invalid cache sharing set for %v mount", m.Type)


We can remove the above m.Type == MountTypeSecret variant of this check entirely, and replace this with:

return nil, errors.Errorf("%s mount should not define sharing set", m.Type)

or similar.

oh, I see I missed your comment; let me know if you want me to look at this

thaJeztah marked this pull request as ready for review October 19, 2022 21:00

aaronlehmann reviewed Oct 21, 2022

View reviewed changes

thaJeztah force-pushed the frontend_cleanup branch from f17cd93 to e5c635a Compare October 21, 2022 19:56

aaronlehmann approved these changes Oct 24, 2022

View reviewed changes

thaJeztah force-pushed the frontend_cleanup branch from e5c635a to 2831c87 Compare December 3, 2022 17:17

jedevc reviewed Dec 5, 2022

View reviewed changes

frontend/dockerfile/instructions/bflag.go Show resolved Hide resolved

frontend/dockerfile/instructions/commands_runmount.go Outdated Show resolved Hide resolved

frontend/dockerfile/instructions/commands_runmount.go Outdated Show resolved Hide resolved

thaJeztah added 6 commits December 5, 2022 13:12

frontend/dockerfile: remove isSSHMountsSupported, isSecretMountsSuppo…

1ae7793

…rted These became obsolete in 906e345, which always enabled them. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

frontend/dockerfile/parser: remove redundant concat

defaaac

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

frontend/dockerfile: provide suggestions for mount share mode

5721d94

similar to df9781b Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah force-pushed the frontend_cleanup branch from 2831c87 to 97cba15 Compare December 5, 2022 12:18

thaJeztah added 4 commits December 5, 2022 13:22

frontend/dockerfile: parseExtraHosts(): use strings.Cut()

a56df1e

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

frontend/dockerfile: BFlags.Parse(): use strings.Cut()

9313bd7

Also some minor cleanup in the boolean value parsing; using a switch instead of if/elseif/else. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah commented Dec 5, 2022

View reviewed changes

thaJeztah force-pushed the frontend_cleanup branch from 97cba15 to 9313bd7 Compare December 5, 2022 12:29

thaJeztah mentioned this pull request Dec 16, 2022

containerd integration: Make build work moby/moby#44079

Merged

crazy-max approved these changes Jan 3, 2023

View reviewed changes

jedevc reviewed Jan 3, 2023

View reviewed changes

jedevc approved these changes Jan 3, 2023

View reviewed changes

jedevc merged commit 60e82c1 into moby:master Jan 12, 2023

thaJeztah deleted the frontend_cleanup branch January 12, 2023 11:53

thaJeztah mentioned this pull request May 24, 2023

containerimage: Export option keys #3694

Merged

		@@ -44,8 +42,7 @@ func TestShellParserMandatoryEnvVars(t *testing.T) {
		require.Equal(t, "", newWord)

Conversation

thaJeztah commented Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

frontend/dockerfile: remove isSSHMountsSupported, isSecretMountsSupported

frontend/dockerfile: parseBuildStageName(): pre-compile regex

frontend/dockerfile/parser: remove redundant concat

frontend/dockerfile/shell: use strings.Equalfold

frontend/dockerfile: define types for enums

frontend/dockerfile: provide suggestions for mount share mode

frontend/dockerfile: move check for cache-sharing

frontend/dockerfile: parseMount() use strings.Cut(), and some minor cleanup

frontend/dockerfile: parseExtraHosts(): use strings.Cut()

frontend/dockerfile: BFlags.Parse(): use strings.Cut()

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aaronlehmann Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah Oct 21, 2022

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 21, 2022

Uh oh!

thaJeztah commented Nov 1, 2022

Uh oh!

thaJeztah commented Dec 4, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

crazy-max left a comment

Choose a reason for hiding this comment

Uh oh!

jedevc Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

thaJeztah commented Oct 19, 2022 •

edited

Loading