-
Notifications
You must be signed in to change notification settings - Fork 68
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
env,unset: support syntax ${VAR_NAME:?}
to error on unset vars
#243
env,unset: support syntax ${VAR_NAME:?}
to error on unset vars
#243
Conversation
LGTM |
}, | ||
ErrFn: func(err error) bool { | ||
return err != nil && strings.Contains(err.Error(), "is not allowed to be unset") | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this test case expected to trigger this error? In its current form, TestBuilder()
can ignore an error, but it can't fail a test that's expecting to generate one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes testcase is expected to trigger the error.
When I alter
- return err != nil && strings.Contains(err.Error(), "is not allowed to be unset")
+ return err != nil && strings.Contains(err.Error(), "is not allowed to be unset hello")
and run
make test
I get expected failure
$ make test
go test ./...
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/README.md] -> /README.md (from: download:true), chown: , chmod
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/LICENSE] -> / (from: download:true), chown: , chmod
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/LICENSE] -> /A (from: download:true), chown: , chmod
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/LICENSE] -> /a (from: download:true), chown: , chmod
2023/02/03 21:10:35 RUN [mkdir ./b] [] true ([])
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/LICENSE] -> /b/a (from: download:true), chown: , chmod
2023/02/03 21:10:35 COPY [https://github.com/openshift/origin/raw/master/LICENSE] -> /b/ (from: download:true), chown: , chmod
2023/02/03 21:10:35 COPY [https://github.com/openshift/ruby-hello-world/archive/master.zip] -> /tmp/ (from: download:true), chown: , chmod
--- FAIL: TestBuilder (0.00s)
--- FAIL: TestBuilder/dockerclient/testdata/Dockerfile.unset_10 (0.00s)
builder_test.go:845: 10: unexpected error: 10: 1: : resolve: Failed to process `${FOO:?}`: FOO is not allowed to be unset
2023/02/03 21:10:35 RUN [useradd -r -s /bin/false -m -d /home/${USER_NAME} ${USER_NAME}] [] true ([USER_NAME=my_user_env])
2023/02/03 21:10:35 ENSURE /home/my_user_env AS "my_user_env"
2023/02/03 21:10:35 RUN [echo $multival] [] true ([multivalarg=a=1 b=2 c=3 d=4 multival=a=1 b=2 c=3 d=4])
FAIL
FAIL github.com/openshift/imagebuilder 0.007s
? github.com/openshift/imagebuilder/cmd/imagebuilder [no test files]
ok github.com/openshift/imagebuilder/dockerclient (cached)
? github.com/openshift/imagebuilder/dockerfile/command [no test files]
ok github.com/openshift/imagebuilder/dockerfile/parser (cached)
? github.com/openshift/imagebuilder/dockerfile/parser/dumper [no test files]
ok github.com/openshift/imagebuilder/imageprogress (cached)
? github.com/openshift/imagebuilder/signal [no test files]
? github.com/openshift/imagebuilder/strslice [no test files]
FAIL
make: *** [Makefile:6: test] Error 1
Therefore I assumed the TestBuilder
is working correctly, could you let me know if i assumed this incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, after #244, if you set the RunErr
field to the error checking callback, we should be able to be sure of that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Docker supports ${SOME_VAR:?} syntax, which will give an error at build if SOME_VAR is unset. ```dockerfile FROM alpine ARG USERID USER ${USERID:?} ``` ```console Failed to process `${USERID:?}`: USERID is not allowed to be unset ``` Closes: containers/buildah#4284 Ref: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02 Signed-off-by: Aditya R <arajan@redhat.com>
4c3b32e
to
ba74835
Compare
@flouthoc: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@nalind PTAL again. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, nalind The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
Docker supports ${SOME_VAR:?} syntax, which will give an error at build if SOME_VAR is unset.
Closes: containers/buildah#4284
Ref: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02