Add build_args support in buildconfig #424

surajnarwade · 2017-02-13T06:50:27Z

now args provided under build in docker-compose file can be available in buildconfig.
it solves #406

k8s-ci-robot · 2017-02-13T06:50:29Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

Once you've signed, please reply here (e.g. "I signed it!") and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.

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.

procrypt · 2017-02-13T07:17:48Z

@surajnarwade Please run gofmt on pkg/transformer/openshift/openshift.go tests are failing.

surajnarwade · 2017-02-13T08:38:58Z

cc: @cdrage @kadel @containscafeine @surajssd

concaf · 2017-02-13T20:24:17Z

pkg/transformer/openshift/openshift.go

+ envList := []kapi.EnvVar{}
+ for i, v := range service.BuildArgs {
+ envList = append(envList, kapi.EnvVar{Name: i, Value: v})
+ }


nit: how about using something like envName or envVal instead of i and v?

concaf · 2017-02-13T20:56:40Z

@surajnarwade works for me, good stuff :)

Let's add some unit and functional tests now.

For the functional ones, you can refer to the other tests in https://github.com/kubernetes-incubator/kompose/blob/master/script/test/cmd/tests.sh, should be straighforward.

For the unit tests, this can be covered under func TestInitBuildConfig(t *testing.T) in openshift_test.go.

Going through the other tests for the first time might take some time, so take some time :)

kadel · 2017-02-14T08:17:25Z

I've tested this and it looks good.

Just one small note here, this doesn't solve entire #406 just one part of it. But that is OK, just don't close #406 yet ;-)

@surajnarwade Can you please add tests to this PR? Mainly Go tests. Otherwise code looks good.

surajnarwade · 2017-02-14T08:39:49Z

@containscafeine @kadel thanks, I am adding tests

surajssd · 2017-02-15T18:42:34Z

script/test/cmd/tests.sh

@@ -177,6 +177,9 @@ convert::check_artifacts_generated "kompose -f $KOMPOSE_ROOT/examples/docker-com
 # Behavior with -o <dirname>/<filename>
 convert::check_artifacts_generated "kompose -f $KOMPOSE_ROOT/examples/docker-compose.yml convert -o $TEMP_DIR/output_file -j" "$TEMP_DIR/output_file"

+# Test the presence of build args in buildconfig
+convert::expect_success_warning "kompose --provider openshift -f $KOMPOSE_ROOT/script/test/fixtures/buildargs/docker-compose.yml convert --stdout -j" "$KOMPOSE_ROOT/script/test/fixtures/buildargs/openshift-buildargs.json" "INFO Buildconfig using [email protected]:surajnarwade/kompose.git::add-build-args as source."


Is info being read as warning here? if function expect_success is failing then we should fix something in that function it seems.

also this needs to be investigated how this test is passing even if we don't have function named: convert::expect_success_warning but it's named as convert::expect_success_and_warning

surajnarwade · 2017-02-22T14:20:46Z

cc @cdrage @kadel

cdrage · 2017-02-22T14:36:51Z

pkg/transformer/openshift/openshift_test.go

 for name, test := range testCases {
 t.Log("Test case: ", name)
 if test.field != test.value {
 t.Errorf("Expected: %#v, got: %#v", test.value, test.field)
 }
 }
+ t.Log("Test case: Assert buildconfig buildargs")


this log should be removed

@surajnarwade This still needs to be removed.

cdrage · 2017-02-22T14:37:26Z

script/test/fixtures/buildargs/build/Dockerfile

+
+
+RUN touch /test
+


why is there a Dockerfile here?

taken from examples for buildconfig

may be best to remove extra newlines from this :)

cdrage · 2017-02-22T14:37:41Z

script/test/fixtures/buildargs/docker-compose.yml

+ args:
+ NAME: web
+ command: "sleep 3600"
+


remove extra newline

@surajnarwade this still needs to be remove

cdrage · 2017-02-22T14:39:12Z

Odd error happening:

===> Starting test <===
convert::expect_success_and_warning: Running: 'kompose --provider openshift -f /home/travis/gopath/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/buildargs/docker-compose.yml convert --stdout -j' expected_output: '/home/travis/gopath/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/buildargs/openshift-buildargs.json' expected_warning: 'Buildconfig using https://github.com/kubernetes-incubator/kompose.git::master as source.'
FAIL: converted output does not match

Any idea?

kadel · 2017-02-23T11:12:29Z

Odd error happening:

 ===> Starting test <===
 convert::expect_success_and_warning: Running: 'kompose --provider openshift -f /home/travis/gopath/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/buildargs/docker-compose.yml convert --stdout -j' expected_output: '/home/travis/gopath/src/github.com/kubernetes-incubator/kompose/script/test/fixtures/buildargs/openshift-buildargs.json' expected_warning: 'Buildconfig using https://github.com/kubernetes-incubator/kompose.git::master as source.'
 FAIL: converted output does not match

Any idea?

I did some digging, and found out that it is caused by this: #445

surajnarwade · 2017-05-05T08:49:44Z

@cdrage my bad, Updated PR for the same

cdrage · 2017-05-09T13:12:22Z

pkg/transformer/openshift/openshift_test.go

 deployapi "github.com/openshift/origin/pkg/deploy/api"

 "github.com/kubernetes-incubator/kompose/pkg/kobject"
 "github.com/kubernetes-incubator/kompose/pkg/testutils"
 "github.com/kubernetes-incubator/kompose/pkg/transformer/kubernetes"
 "github.com/pkg/errors"
+ kapi "k8s.io/kubernetes/pkg/api"


errr.. why are we importing this twice? "k8s.io/kubernetes/pkg/api" is already imported

cdrage · 2017-05-09T13:12:30Z

pkg/transformer/openshift/openshift.go

@@ -200,6 +200,13 @@ func (o *OpenShift) initImageStream(name string, service kobject.ServiceConfig,
 // initBuildConfig initialize Openshifts BuildConfig Object
 func initBuildConfig(name string, service kobject.ServiceConfig, repo string, branch string) (*buildapi.BuildConfig, error) {
 contextDir, err := getAbsBuildContext(service.Build)
+ envList := []kapi.EnvVar{}


no need to use kapi

as #602 puts kapi instead of api

Alright, i see 👍 LGTM

cdrage · 2017-05-09T13:12:41Z

pkg/transformer/openshift/openshift.go

+ if *envValue == "\x00" {
+ *envValue = os.Getenv(envName)
+ }
+ envList = append(envList, kapi.EnvVar{Name: envName, Value: *envValue})


no need to use kapi, use api

reason: #602

cdrage · 2017-05-09T13:13:11Z

pkg/transformer/openshift/openshift_test.go

@@ -288,9 +289,12 @@ func TestInitBuildConfig(t *testing.T) {
 serviceName := "serviceA"
 repo := "https://git.test.com/org/repo"
 branch := "somebranch"
+ buildArgs := []kapi.EnvVar{{Name: "name", Value: "value"}}


again, use just api no kapi

cdrage · 2017-05-09T13:13:58Z

pkg/transformer/openshift/openshift_test.go

 for name, test := range testCases {
 t.Log("Test case: ", name)
 if test.field != test.value {
 t.Errorf("Expected: %#v, got: %#v", test.value, test.field)
 }
 }
+ t.Log("Test case: Assert buildconfig buildargs")
+ if !reflect.DeepEqual(bc.Spec.CommonSpec.Strategy.DockerStrategy.Env, buildArgs) {


Bit confused what this does, can you please elaborate with a comment above?

cdrage · 2017-05-09T13:15:15Z

script/test/fixtures/buildargs/docker-compose.yml

+ context: "./build"
+ args:
+ NAME: web
+ command: "sleep 3600"


Would be good to test all combinations, ex:

args: NAME: web

args: - NAME=web

args: - NAME

args: NAME:

@surajnarwade are all those combinations added?

@surajssd , yes

surajnarwade · 2017-05-15T09:16:13Z

cc @cdrage

cdrage · 2017-05-15T15:07:31Z

script/test/cmd/tests.sh

@@ -235,6 +239,6 @@ convert::expect_success "kompose --provider=openshift convert --stdout -j" "$KOM
 cd $CURRENT_DIR

 # Removes generated output
-rm -rf /tmp/output-os.json
+rm -rf /tmp/output*.json


recursive and force plus a * = bad idea. very dangerous.
instead, please add:

rm /tmp/output-os.json rm /tmp/output-buildarg-os.json

cdrage · 2017-05-15T15:07:54Z

script/test/fixtures/buildargs/docker-compose.yml

+ args:
+ NAME: web
+ command: "sleep 3600"
+


@surajnarwade this still needs to be remove

cdrage · 2017-05-15T15:08:27Z

script/test/fixtures/buildargs/docker-compose.yml

+ context: "./build"
+ args:
+ - NAME=web
+ - foo


no tests for:

args: NAME:

?

@cdrage , this type is not supported in build args, and as buildargs is of map[string]*string, it throws error,

ERRO Failed to unmarshall: Cannot unmarshal '<nil>' to type <nil> into a string value

and this format is not supported by libcompose

@surajnarwade Alright, no worries. Despite it being valid YAML, it's something that's not listed on https://docs.docker.com/compose/compose-file/compose-file-v2/#args 👍 LGTM!

surajssd · 2017-05-15T16:28:12Z

script/test/fixtures/buildargs/README.md

@@ -0,0 +1,12 @@
+## Docker Compose Buildargs
+


nit: s//n// remove extra new line!

cdrage · 2017-05-15T17:12:15Z

After all the changes, this LGTM 👍

surajssd · 2017-05-15T17:32:40Z

pkg/transformer/openshift/openshift_test.go

@@ -17,13 +17,13 @@ limitations under the License.
 package openshift

 import (
+ "k8s.io/kubernetes/pkg/api"


can we remain consistent with openshit.go and use the alias for k8s api import as kapi ?

@surajssd okay

surajssd · 2017-05-16T04:50:59Z

script/test/fixtures/buildargs/docker-compose.yml

+ NAME: web
+ command: "sleep 3600"
+ foo1:
+ build:


nit: why is the indentation of this docker-compose file messed up?

now args provided under build in docker-compose file can be available in buildconfig. it solves kubernetes#406 Added unit test and functional test solves kubernetes#445 Separated key:"value" pairs by spaces

surajssd · 2017-05-16T08:14:48Z

Cool this LGTM!

surajssd · 2017-05-16T08:16:20Z

@surajnarwade thanks for the hard work!

surajnarwade · 2017-05-16T08:36:15Z

@surajssd, finally :)

arcolife · 2017-05-16T17:40:47Z

thanks everyone!

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 13, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 13, 2017

surajnarwade force-pushed the add-build-args branch 2 times, most recently from 9ed1b03 to 2e9596c Compare February 13, 2017 08:25

concaf reviewed Feb 13, 2017

View reviewed changes

surajssd added the component/OpenShift label Feb 14, 2017

surajnarwade force-pushed the add-build-args branch 4 times, most recently from 65b1d21 to 33e97cb Compare February 15, 2017 15:53

surajssd reviewed Feb 15, 2017

View reviewed changes

surajnarwade force-pushed the add-build-args branch from 33e97cb to 391a112 Compare February 21, 2017 07:32

cdrage requested changes Feb 22, 2017

View reviewed changes

kadel added the rebase needed label Feb 22, 2017

surajnarwade force-pushed the add-build-args branch from 391a112 to f60c6ef Compare February 23, 2017 11:05

surajnarwade force-pushed the add-build-args branch 5 times, most recently from 74c8136 to 95e469c Compare February 25, 2017 19:55

cdrage reviewed May 9, 2017

View reviewed changes

surajnarwade force-pushed the add-build-args branch 5 times, most recently from 9095ab8 to c3f529f Compare May 9, 2017 14:29

cdrage requested changes May 15, 2017

View reviewed changes

surajssd reviewed May 15, 2017

View reviewed changes

surajnarwade force-pushed the add-build-args branch 2 times, most recently from a79f9a5 to 32d84e5 Compare May 15, 2017 16:55

cdrage approved these changes May 15, 2017

View reviewed changes

surajssd reviewed May 15, 2017

View reviewed changes

surajnarwade force-pushed the add-build-args branch from 32d84e5 to 708a41a Compare May 15, 2017 17:35

surajssd reviewed May 16, 2017

View reviewed changes

surajnarwade force-pushed the add-build-args branch from 708a41a to 6f34dfa Compare May 16, 2017 05:54

Add build_args support in buildconfig

8fc262b

now args provided under build in docker-compose file can be available in buildconfig. it solves kubernetes#406 Added unit test and functional test solves kubernetes#445 Separated key:"value" pairs by spaces

surajnarwade force-pushed the add-build-args branch from 6f34dfa to 8fc262b Compare May 16, 2017 06:49

surajssd merged commit 7e785bb into kubernetes:master May 16, 2017

Add build_args support in buildconfig #424

Add build_args support in buildconfig #424

Conversation

surajnarwade commented Feb 13, 2017

k8s-ci-robot commented Feb 13, 2017

procrypt commented Feb 13, 2017 • edited Loading

surajnarwade commented Feb 13, 2017

Choose a reason for hiding this comment

concaf commented Feb 13, 2017 • edited Loading

kadel commented Feb 14, 2017

surajnarwade commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented Feb 22, 2017

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

cdrage commented Feb 22, 2017

kadel commented Feb 23, 2017

surajnarwade commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage May 9, 2017 • edited Loading

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

cdrage May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade commented May 15, 2017

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

cdrage commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajssd commented May 16, 2017

surajssd commented May 16, 2017

surajnarwade commented May 16, 2017

arcolife commented May 16, 2017

procrypt commented Feb 13, 2017 •

edited

Loading

concaf commented Feb 13, 2017 •

edited

Loading

cdrage May 9, 2017 •

edited

Loading

cdrage May 9, 2017 •

edited

Loading