Error when user gives build args with non-Docker strat #13257
Conversation
pkg/cmd/cli/cmd/newapp.go
Outdated
|
|
||
| if len(config.BuildArgs) > 0 && config.Strategy != generate.StrategyDocker { | ||
| return kcmdutil.UsageError(c, "build arguments can only be used with Docker builds.") | ||
| } |
There was a problem hiding this comment.
do we necessarily know the strategy that's going to be used at this point? what is the behavior when no strategy is explicitly specified?
There was a problem hiding this comment.
You're right, I didn't catch generate.StrategyUnspecified. I've added that to the check. Do we want to error out if the Build doesn't use the BuildArgs? Currently, the BuildArgs will not be used if no ComponentReferences have a DockerStrategy.
However, since we'll likely only have one SourceRepository while using new-build, we could throw an error if the Strategy for that is not DockerStrategy.
There was a problem hiding this comment.
i have mixed feelings as it is a slippery slope, but yes i guess if someone explicitly indicated they want to use build-args, we should throw an error if we aren't going to produce a docker build object.
|
also a change like this should have a test added so we don't regress it later. |
|
@bparees Added a test, and the check for unspecified strategy, ptal? |
test/cmd/newapp.sh
Outdated
| @@ -183,6 +183,9 @@ os::cmd::expect_failure_and_text "echo 'S P A C E S=test' | oc new-build --binar | |||
| # new-build - check that we can set build-args in DockerStrategy | |||
| os::cmd::expect_success_and_text "oc new-build ${OS_ROOT}/test/testdata/build-arg-dockerfile --build-arg 'foo=bar' --to 'test' -o jsonpath='{.items[?(@.kind==\"BuildConfig\")].spec.strategy.dockerStrategy.buildArgs[?(@.name==\"foo\")].value}'" 'bar' | |||
|
|
|||
| # check that we cannot set build-args in a non-DockerStrategy build | |||
| os::cmd::expect_failure "oc new-build https://github.com/openshift/ruby-hello-world --strategy=source --build-arg 'foo=bar'" | |||
There was a problem hiding this comment.
should also run it w/ the strategy unspecified.
|
@bparees Ready for another lookover, ptal? Added another test, and a check during the buildPipeline step |
pkg/generate/app/cmd/newapp.go
Outdated
| // needs to be modified to handle that. | ||
| if len(c.BuildArgs) > 0 && refInput.Uses.GetStrategy() != generate.StrategyDocker { | ||
| return nil, fmt.Errorf("Cannot use build arguments, not a Docker build") | ||
| } |
There was a problem hiding this comment.
why not just iterate the objects at the end? if build-args was passed, and at least one of the objects being created is not a buildconfig w/ a docker strategy, throw an error.
|
Flake #13271 |
|
@csrwng Since Ben is on PTO, ptal? |
pkg/generate/app/cmd/newapp.go
Outdated
| return nil, fmt.Errorf("Cannot use build arguments without a Docker build") | ||
| } | ||
| if numDockerBuilds > 1 { | ||
| fmt.Fprintf(c.ErrOut, "--> WARNING: Using applying build arguments to multiple Docker builds.\n") |
pkg/generate/app/cmd/newapp.go
Outdated
|
|
||
| if len(c.BuildArgs) > 0 { | ||
| if numDockerBuilds == 0 { | ||
| return nil, fmt.Errorf("Cannot use build arguments without a Docker build") |
There was a problem hiding this comment.
is this the error that gets reported to the user? "build arguments" is probably too vague (all arguments to "oc new-build" are build arguments). Specify --build-args (or whatever the flag name is) instead.
test/cmd/newapp.sh
Outdated
| @@ -193,6 +193,10 @@ os::cmd::expect_failure_and_text "echo 'S P A C E S=test' | oc new-build --binar | |||
| # new-build - check that we can set build-args in DockerStrategy | |||
| os::cmd::expect_success_and_text "oc new-build ${OS_ROOT}/test/testdata/build-arg-dockerfile --build-arg 'foo=bar' --to 'test' -o jsonpath='{.items[?(@.kind==\"BuildConfig\")].spec.strategy.dockerStrategy.buildArgs[?(@.name==\"foo\")].value}'" 'bar' | |||
|
|
|||
| # check that we cannot set build-args in a non-DockerStrategy build | |||
| os::cmd::expect_failure "oc new-build https://github.com/openshift/ruby-hello-world --strategy=source --build-arg 'foo=bar'" | |||
| os::cmd::expect_failure "oc new-build https://github.com/openshift/ruby-ex --build-arg 'foo=bar'" | |||
There was a problem hiding this comment.
use expect_failure_and_text for both of these. make sure you're getting the error you expect.
|
Flake #13288, [test] |
|
Evaluated for origin test up to 7e52d29 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/49/) (Base Commit: a4efef0) |
|
[merge] |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/49/) (Base Commit: a4efef0) (Image: devenv-rhel7_6058) |
|
Evaluated for origin merge up to 7e52d29 |
Adds a new error message when a user specifies Docker build-args on a non-Docker build.
Fixes bz1429326
Related Trello card here
@openshift/devex @bparees @dongboyan77 ptal?
[test]