Use new crd-schema-gen in Makefile #514

damemi · 2019-07-01T20:46:45Z

This is an example of how we could use the crd-schema-gen hosted image in our makefiles, and ultimately remove the dependency on the library-go scripts which were based on hacked and patched forks of dependencies. Ideally I think we could just add these commands to library-go's alpha-build-machinery helpers and import from there.

damemi · 2019-07-01T20:56:06Z

Note that this won't work right now because of openshift/api#362

Makefile

damemi · 2019-07-02T15:38:21Z

make: podman: Command not found, also tried with docker, what command should I use to run containers in makefiles?

tnozicka · 2019-07-03T05:26:38Z

It's not about makefiles, but CI infra. Given our tests run as containers, it sounds like you are trying to do nested docker which you can't (easily), it may be possible with podman

We'd need to extend the golang image with podman in a step or ask it to be included there (which I am not sure of, but there are already other tools there)
https://github.com/openshift/release/blob/master/ci-operator/config/openshift/cluster-kube-apiserver-operator/openshift-cluster-kube-apiserver-operator-master.yaml#L7

I'd like to see the nested podman tested manually first

damemi · 2019-07-08T17:07:31Z

bumped this to pull in API fixes and ran the new generator, looks like the missing top-level descriptions are there now but this also seemed to generate validations for ObjectMeta, which was apparently missing before. Does that look right?

damemi · 2019-07-08T19:00:26Z

/retest

damemi · 2019-07-08T21:31:49Z

/retest

Makefile

sttts · 2019-07-09T17:00:58Z

Makefile

@@ -44,8 +44,10 @@ PKG := ${PWD:${GOPATH}/%=%}
 update-codegen-crds:
 	docker run -v ${PWD}:/go/$(PKG):Z -w /go/$(PKG) registry.svc.ci.openshift.org/ocp/4.2:crd-schema-gen --apis-dir vendor/github.com/openshift/api/operator/v1
 update-codegen: update-codegen-crds
+
+TMP_GOPATH :=$(shell mktemp -d)


who deletes it?

sttts · 2019-07-09T17:02:03Z

Makefile

 verify-codegen-crds:
-	go get github.com/openshift/crd-schema-gen/... && go install github.com/openshift/crd-schema-gen/...
+	export GOPATH=$(TMP_GOPATH) && export GOBIN=$(TMP_GOPATH)/bin && git clone -b release-4.2 --single-branch --depth 1 https://github.com/openshift/crd-schema-gen.git $(TMP_GOPATH)/src/github.com/openshift/crd-schema-gen && go install $(TMP_GOPATH)/src/github.com/openshift/crd-schema-gen/cmd/crd-schema-gen


GOPATH=$(TMP_GOPATH) GOBIN=... go install ... is enough. No need to export. git does not use them.

sure about GOBIN? We have to call it from there as well.

some people like @stlaz have set GOBIN explicitly and without setting it to temp we install it to the global GOBIN instead the one in temporary GOPATH

sttts · 2019-07-09T17:16:09Z

Makefile

 verify-codegen-crds:
-	go run ./vendor/github.com/openshift/library-go/cmd/crd-schema-gen/main.go --domain openshift.io --apis-dir vendor/github.com/openshift/api --verify-only
+	git clone -b release-4.2 --single-branch --depth 1 https://github.com/openshift/crd-schema-gen.git $(TMP_GOPATH)/src/github.com/openshift/crd-schema-gen


thanks for the line breaks :)

Makefile

tnozicka · 2019-07-09T17:23:10Z

Makefile

@@ -44,8 +44,10 @@ PKG := ${PWD:${GOPATH}/%=%}
 update-codegen-crds:
 	docker run -v ${PWD}:/go/$(PKG):Z -w /go/$(PKG) registry.svc.ci.openshift.org/ocp/4.2:crd-schema-gen --apis-dir vendor/github.com/openshift/api/operator/v1
 update-codegen: update-codegen-crds
+
+TMP_GOPATH :=$(shell mktemp -d)


tnozicka · 2019-07-09T17:23:29Z

Makefile

+crd-schema-gen:
+	git clone -b release-4.2 --single-branch --depth 1 https://github.com/openshift/crd-schema-gen.git $(TMP_GOPATH)/src/github.com/openshift/crd-schema-gen
+	GOPATH=$(TMP_GOPATH) go install $(TMP_GOPATH)/src/github.com/openshift/crd-schema-gen/cmd/crd-schema-gen
+	crd-schema-gen --apis-dir vendor/github.com/openshift/api/operator/v1


by convention we end phony target with `.PHONY: target-name

I see that was preexisting, so it's up to you to fix it

Is adding it to the .PHONY list at the end good?

it works the same way, our convention is to use it as {} to scope the target

Makefile

tnozicka

the makefile structure looks good now, thx

tnozicka · 2019-07-09T18:57:34Z

Makefile

@@ -40,11 +40,18 @@ test-e2e: GO_TEST_PACKAGES :=./test/e2e/...
 test-e2e: GO_TEST_FLAGS += -v
 test-e2e: test-unit

-update-codegen-crds:
-	go run ./vendor/github.com/openshift/library-go/cmd/crd-schema-gen/main.go --domain openshift.io --apis-dir vendor/github.com/openshift/api
+crd-schema-gen:


install-crd-schema-gen?

Makefile

sttts · 2019-07-09T21:00:45Z

/lgtm
/approve

openshift-ci-robot · 2019-07-09T21:01:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, sttts

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:

~~OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

damemi · 2019-07-09T22:33:03Z

/test e2e-aws

damemi requested a review from sttts July 1, 2019 20:46

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2019

openshift-ci-robot requested a review from deads2k July 1, 2019 20:47

damemi mentioned this pull request Jul 1, 2019

ProxyArguments being map[string][]string causes an error in kubebuilder CRD generator openshift/api#362

Closed

sttts reviewed Jul 1, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

damemi force-pushed the crd-gen-makefile branch 2 times, most recently from 4db1e86 to ba1adbb Compare July 2, 2019 15:23

damemi force-pushed the crd-gen-makefile branch 2 times, most recently from 5ddf565 to f5929ed Compare July 8, 2019 17:02

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 8, 2019

adambkaplan mentioned this pull request Jul 8, 2019

Configure the Service Ingress IP Controller (AutoExternalIPs) openshift/cluster-openshift-controller-manager-operator#104

Merged

damemi force-pushed the crd-gen-makefile branch from f5929ed to d44e933 Compare July 8, 2019 19:15

sttts reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

sttts reviewed Jul 9, 2019

View reviewed changes

damemi force-pushed the crd-gen-makefile branch from 7506a65 to b0f4f97 Compare July 9, 2019 17:13

sttts reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

tnozicka reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

damemi force-pushed the crd-gen-makefile branch from b0f4f97 to 84a7f25 Compare July 9, 2019 17:21

tnozicka suggested changes Jul 9, 2019

View reviewed changes

damemi force-pushed the crd-gen-makefile branch 2 times, most recently from 8b9737e to a513b43 Compare July 9, 2019 17:54

damemi force-pushed the crd-gen-makefile branch from a513b43 to 9ba4b9b Compare July 9, 2019 17:56

tnozicka reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

damemi force-pushed the crd-gen-makefile branch from 9ba4b9b to 2af8940 Compare July 9, 2019 18:51

tnozicka reviewed Jul 9, 2019

View reviewed changes

damemi changed the title ~~Use crd-schema-gen image in Makefile~~ Use new crd-schema-gen in Makefile Jul 9, 2019

damemi force-pushed the crd-gen-makefile branch 3 times, most recently from 08b71d9 to d66e166 Compare July 9, 2019 19:44

sttts reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

sttts reviewed Jul 9, 2019

View reviewed changes

Makefile Outdated Show resolved Hide resolved

damemi added 4 commits July 9, 2019 16:49

Use crd-schema-gen image in Makefile

8dba590

Remove controller-tools fork dep

a298a2e

bump(*): api changes for crd-gen fix

5b6f033

make update-codegen

6136831

damemi force-pushed the crd-gen-makefile branch from d66e166 to 6136831 Compare July 9, 2019 20:49

openshift-ci-robot assigned sttts Jul 9, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2019

openshift-merge-robot merged commit b523635 into openshift:master Jul 9, 2019

This was referenced Jul 10, 2019

Use new crd-schema-gen Makefile pattern openshift/cluster-openshift-apiserver-operator#210

Merged

Use new crd-schema-gen Makefile pattern openshift/cluster-kube-controller-manager-operator#265

Merged

damemi deleted the crd-gen-makefile branch July 11, 2019 14:57

damemi mentioned this pull request Jul 11, 2019

Add spec to required fields in manifest #520

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new crd-schema-gen in Makefile #514

Use new crd-schema-gen in Makefile #514

damemi commented Jul 1, 2019

damemi commented Jul 1, 2019

damemi commented Jul 2, 2019

tnozicka commented Jul 3, 2019 •

edited

Loading

damemi commented Jul 8, 2019

damemi commented Jul 8, 2019

damemi commented Jul 8, 2019

sttts Jul 9, 2019

tnozicka Jul 9, 2019

sttts Jul 9, 2019

sttts Jul 9, 2019

tnozicka Jul 9, 2019

sttts Jul 9, 2019

tnozicka Jul 9, 2019

tnozicka Jul 9, 2019

tnozicka Jul 9, 2019

damemi Jul 9, 2019

tnozicka Jul 9, 2019

tnozicka left a comment

tnozicka Jul 9, 2019

sttts commented Jul 9, 2019

openshift-ci-robot commented Jul 9, 2019

damemi commented Jul 9, 2019

Use new crd-schema-gen in Makefile #514

Use new crd-schema-gen in Makefile #514

Conversation

damemi commented Jul 1, 2019

damemi commented Jul 1, 2019

damemi commented Jul 2, 2019

tnozicka commented Jul 3, 2019 • edited Loading

damemi commented Jul 8, 2019

damemi commented Jul 8, 2019

damemi commented Jul 8, 2019

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jul 9, 2019

openshift-ci-robot commented Jul 9, 2019

damemi commented Jul 9, 2019

tnozicka commented Jul 3, 2019 •

edited

Loading