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.

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

Trimming in variable substitution #4287

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

tstenner
Copy link

Add #, ##, % and %% to variable substitution (inspired by #3611 and the almost identical shell functionality).

Can be used like this:

ARG IMAGE=busybox:latest
ARG VERSION=1.2.3
# replace tag
FROM ${IMAGE%:*}:stable
ENV MAJOR_MINOR_VERSION=${VERSION%.*}
ENV MAJOR_VERSION=${VERSION%%.*}

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

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

Design LGTM

@jsternberg Could you verify the replacement algorithm?

fyi @thaJeztah @tianon

frontend/dockerfile/docs/reference.md Outdated Show resolved Hide resolved
@tonistiigi
Copy link
Member

Copy link
Collaborator

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

I'm recommending a different method of creating the regular expression. As mentioned in one of the comments, I find constructing regular expressions by using strings.Replace and regexp.QuoteMeta to be hard to reason about and verify. Since filepath matches and regular expressions are both types of regular expressions, it would be easier and more reliable to scan the string and emit the appropriate character. It's also easier to verify the behavior as correct since you don't have to think about "was this already escaped?" or "do I have to match the escaped version?" Each character has a simple and direct translation.

frontend/dockerfile/shell/lex.go Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex_test.go Outdated Show resolved Hide resolved
@tstenner
Copy link
Author

tstenner commented Oct 2, 2023

Many thanks for the detailled feedback!

It's currently a long holiday weekend so I haven't tested it one real world Dockerfiles yet, but the code should be ready and the unit tests cover quite a lot more.

@jsternberg
Copy link
Collaborator

It looks much better. I'm working on a second pass of a review, but got delayed and need some time to think about some recommendations. I did want to give a high-level overview of some things that were concerning me.

  1. The reverse regex is a big concern for me. I understand what's being done and why, but also the logic seems a bit too much. I've been trying to think of some better ways to do it that aren't more confusing in their own right.
  2. I'm also not sure I understand the bidi package. It seems to me like it's a new experimental package for unicode readers to understand the directionality in the text for different languages. I think I understand the use of this, but it doesn't seem that useful in practice for this specific case. I'd probably be generally more comfortable with using the strings, unicode/utf8, and/or bytes package or just handling the string or []rune directly. The API is also listed as experimental. It doesn't really seem like that specific function would be one that's likely to change, but it's sole use is for reversing the string.

frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Show resolved Hide resolved
frontend/dockerfile/shell/lex.go Outdated Show resolved Hide resolved
@jedevc
Copy link
Member

jedevc commented Oct 5, 2023

Could we also get some some more general tests in dockerfile_test.go? We don't need to test all the possible edge case behaviour (that can be done in the lex_test.go), but it would good to at least have some sanity checks as well, just to check it works in the actual Dockerfile!

@tstenner thanks so much for looking at this, nice to see someone following up on #3611 🎉

@tstenner
Copy link
Author

tstenner commented Oct 9, 2023

@jedevc I expanded the one test for variable substitution to also include a basic test (trimming a constant string from the left) and a worst-case trim (i.e. longest match from the right) in e5b8134.

Copy link
Collaborator

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

Squash and get the tests working and I think this is good.

There's one additional change that I was thinking about, but I don't feel strongly enough about it at the moment to ask you to do it so I'll just mark it down here as a potential future change.

frontend/dockerfile/shell/lex.go Show resolved Hide resolved
@tstenner
Copy link
Author

I rebased + squashed the code and the non-pushing pipelines are green in my branch. Someone with more rights than I have could go ahead and trigger the CI pipeline and hope for the best.

@thaJeztah
Copy link
Member

I gave CI a kick to run

@tstenner
Copy link
Author

There's one check failing, but I don't see what this PR could have to do with it:

2023-10-20T13:52:05.6780866Z === FAIL: frontend/dockerfile TestIntegration/TestRunGlobalNetwork/worker=containerd-snapshotter-stargz/frontend=gateway/network.host=granted (7.92s)
2023-10-20T13:52:05.6782189Z     dockerfile_runnetwork_test.go:190: 
2023-10-20T13:52:05.6782946Z         	Error Trace:	/src/frontend/dockerfile/dockerfile_runnetwork_test.go:190
2023-10-20T13:52:05.6783759Z         	            				/src/util/testutil/integration/run.go:91
2023-10-20T13:52:05.6784414Z         	            				/src/util/testutil/integration/run.go:205
2023-10-20T13:52:05.6785038Z         	Error:      	Received unexpected error:
2023-10-20T13:52:05.6786027Z         	            	process "/bin/sh -c ! nc -z 127.0.0.1 39207" did not complete successfully: exit code: 1
2023-10-20T13:52:05.6786925Z         	            	github.com/moby/buildkit/util/stack.Enable
2023-10-20T13:52:05.6787535Z         	            		/src/util/stack/stack.go:77
2023-10-20T13:52:05.6788169Z         	            	github.com/moby/buildkit/util/grpcerrors.FromGRPC
2023-10-20T13:52:05.6788861Z         	            		/src/util/grpcerrors/grpcerrors.go:198
2023-10-20T13:52:05.6789662Z         	            	github.com/moby/buildkit/util/grpcerrors.UnaryClientInterceptor
2023-10-20T13:52:05.6790476Z         	            		/src/util/grpcerrors/intercept.go:41
2023-10-20T13:52:05.6791122Z         	            	google.golang.org/grpc.(*ClientConn).Invoke
2023-10-20T13:52:05.6791791Z         	            		/src/vendor/google.golang.org/grpc/call.go:35
2023-10-20T13:52:05.6793008Z         	            	github.com/moby/buildkit/api/services/control.(*controlClient).Solve
2023-10-20T13:52:05.6793861Z         	            		/src/api/services/control/control.pb.go:2208
2023-10-20T13:52:05.6794574Z         	            	github.com/moby/buildkit/client.(*Client).solve.func2
2023-10-20T13:52:05.6795234Z         	            		/src/client/solve.go:258
2023-10-20T13:52:05.6795839Z         	            	golang.org/x/sync/errgroup.(*Group).Go.func1
2023-10-20T13:52:05.6796546Z         	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
2023-10-20T13:52:05.6797187Z         	            	runtime.goexit
2023-10-20T13:52:05.6797792Z         	            		/usr/local/go/src/runtime/asm_amd64.s:1650
2023-10-20T13:52:05.6798359Z         	            	failed to solve
2023-10-20T13:52:05.6798967Z         	            	github.com/moby/buildkit/client.(*Client).solve.func2
2023-10-20T13:52:05.6799812Z         	            		/src/client/solve.go:273
2023-10-20T13:52:05.6800426Z         	            	golang.org/x/sync/errgroup.(*Group).Go.func1
2023-10-20T13:52:05.6801145Z         	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
2023-10-20T13:52:05.6801772Z         	            	runtime.goexit
2023-10-20T13:52:05.6802313Z         	            		/usr/local/go/src/runtime/asm_amd64.s:1650
2023-10-20T13:52:05.6803716Z         	Test:       	TestIntegration/TestRunGlobalNetwork/worker=containerd-snapshotter-stargz/frontend=gateway/network.host=granted

Copy link
Collaborator

@dvdksn dvdksn left a comment

Choose a reason for hiding this comment

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

Suggest some rewording and adding examples (the list format is getting unwieldy here but I think this can work for now)

frontend/dockerfile/docs/reference.md Outdated Show resolved Hide resolved
frontend/dockerfile/docs/reference.md Outdated Show resolved Hide resolved
frontend/dockerfile/docs/reference.md Outdated Show resolved Hide resolved
@tstenner
Copy link
Author

@dvdksn Thanks for the documentation proposal. I have merged + squashed them.

@tonistiigi
Copy link
Member

Added a commit to make it clear what syntax is new. Reverted the change in the code example. If we want to change the code example, then it should not use the new syntax and can therefore be a separate PR, or there should be a separate code example for the new syntax. PTAL

@tstenner
Copy link
Author

tstenner commented Nov 1, 2023

@tonistiigi Sounds good. I also applied @dvdksn's suggestions. Should I rebase them or will they get squashed anyway?

@jedevc
Copy link
Member

jedevc commented Nov 1, 2023

@tstenner we don't squash commits during merge, so we can preserve history (it's so much easier to look through a git-blame when each commit is a single piece of work, instead of having to constantly go look at github).

If you could rebase, that would be excellent ❤️

Review comments and edge cases

- the `${}` parser handles escapes, but needs to preserve them for `#`/`%`
  - but `\}` needs to be de-escaped
- reversing strings need to handle escapes, i.e. `a\*c` -> `c\*a`
- build the regex with a scanner, not QuoteMeta+StringReplace
- add more complicated cases to the tests

Separate out + unit test helper functions

Add trim test to dockerfile_test

Signed-off-by: Tristan Stenner <[email protected]>
@tstenner
Copy link
Author

tstenner commented Nov 1, 2023

@jedevc I rebased and squashed everything.

@tstenner
Copy link
Author

Apart from two (unrelated?) CI failures everything seems fine. Is there anything left for me to do?

@tonistiigi tonistiigi merged commit 4204061 into moby:master Nov 16, 2023
57 of 58 checks passed
@tonistiigi
Copy link
Member

@tstenner Thanks!

@tstenner tstenner deleted the variable_expansion_trimming branch November 16, 2023 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants