move builder images to use external apis #17699

bparees · 2017-12-08T18:45:17Z

No description provided.

bparees · 2017-12-08T19:42:49Z

tools/import-verifier/import-verifier.go

@@ -215,14 +215,14 @@ func main() {

 		// make sure that all the allowed imports are used
 		if unused := unusedPackageImports(restriction.AllowedImportPackages, packages); len(unused) > 0 {
-			log.Printf("-- found unused package imports\n")
+			log.Printf("-- found unused allowed package imports(remove them)\n")


@deads2k

heh. Sure.

bparees · 2017-12-11T20:15:19Z

/retest

bparees · 2017-12-11T20:32:57Z

@openshift/sig-developer-experience @deads2k ptal. Note that I will be relocating the new/duplicated constants from their present "utils" location into the external api types file once the dust settles from the rebase.

gabemontero

lgtm

One question: I saw the verify test passed ... just curious about the need for make update type changes; were they done previously, when the new api packages were first introduced, and simply referencing them here in pkg/build is orthogonal?

bparees · 2017-12-13T14:52:11Z

just curious about the need for make update type changes; were they done previously, when the new api packages were first introduced, and simply referencing them here in pkg/build is orthogonal?

any time you pull external api changes into origin you need to run make update as part of your PR because the conversion code (between the external types and internal types) lives in origin and must be updated.

deads2k · 2017-12-15T20:20:12Z

pkg/build/builder/cmd/builder.go

-	obj, groupVersionKind, err := legacyscheme.Codecs.UniversalDecoder().Decode([]byte(buildStr), nil, nil)
+	cfg.build = &buildapiv1.Build{}
+
+	obj, groupVersionKind, err := legacyscheme.Codecs.UniversalDecoder().Decode([]byte(buildStr), nil, cfg.build)


The legacy scheme keeps you pinned to the internal versions to operate. You'll need a follow up to remove the dependency. Definitely not a blocker.

is there a different decoder/scheme I should be using instead?

deads2k · 2017-12-15T20:20:36Z

pkg/build/builder/cmd/builder.go

@@ -73,11 +71,8 @@ func newBuilderConfigFromEnvironment(out io.Writer, needsDocker bool) (*builderC
 		}
 		glog.V(4).Infof("redacted build: %v", string(bytes))
 	}
-	if errs := validation.ValidateBuild(cfg.build); len(errs) > 0 {


Interesting. What did you want this protection for?

I think it was belt+suspenders. It sort of confirmed that the build object we serialized into the env variable was in fact still valid when we deserialized it on the other side. The build object would have already been through validation when it was created in the first place. Given how impossible it would have been to retain this check(since validation runs against internal apis), I opted for removing the additional protection.

I think it was belt+suspenders. It sort of confirmed that the build object we serialized into the env variable was in fact still valid when we deserialized it on the other side. The build object would have already been through validation when it was created in the first place. Given how impossible it would have been to retain this check(since validation runs against internal apis), I opted for removing the additional protection.

Ok. In general, client-side validation on something like this is questionable since the client rules are unlikely to consistently match the server rules.

right, well prior to splitting things out it was using the same rules(same code). but now it's being removed so it's moot.

deads2k · 2017-12-15T20:23:15Z

pkg/build/builder/timing/context.go

 	stages := fromContext(ctx)
 	return *stages
 }
+
+// RecordStageAndStepInfo records details about each build stage and step
+func RecordStageAndStepInfo(stages []buildapiv1.StageInfo, stageName buildapiv1.StageName, stepName buildapiv1.StepName, startTime metav1.Time, endTime metav1.Time) []buildapiv1.StageInfo {


Straight move?

other than changing it to use external api objects, yeah.

deads2k · 2017-12-15T20:24:27Z

pkg/build/builder/util/consts.go

+
+	// OriginVersion is an environment variable key that indicates the version of origin that
+	// created this build definition.
+	OriginVersion = "ORIGIN_VERSION"


Are these guaranteed in any way for custom builds?

sort of? we document it here:
https://docs.openshift.org/latest/creating_images/custom.html#custom-builder-image

sort of? we document it here:
https://docs.openshift.org/latest/creating_images/custom.html#custom-builder-image

That makes it sound a lot like an API constant. Doesn't need to move here, but if it is guaranteed, we can't change it, and we expect clients to consume it, then it is a good target for moving to openshift/api.

deads2k · 2017-12-15T20:26:58Z

Very minor comments, nothing that would keep me from merging this as-is (minus the bump thing).

lgtm

bparees · 2017-12-19T01:05:18Z

/retest

bparees · 2017-12-20T00:45:01Z

/retest

openshift-ci-robot · 2018-01-03T15:11:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [bparees]
~~pkg/build/OWNERS~~ [bparees]
~~tools/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

bparees · 2018-01-03T16:11:05Z

#17970

/retest

bparees · 2018-01-04T03:42:16Z

/retest

openshift-bot · 2018-01-05T07:15:35Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2018-01-05T09:21:54Z

/test all [submit-queue is verifying that this PR is safe to merge]

openshift-ci-robot · 2018-01-05T10:48:51Z

@bparees: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_image_ecosystem	`05adf6e`	link	`/test extended_image_ecosystem`
ci/openshift-jenkins/extended_conformance_gce	`66c9d91`	link	`/test extended_conformance_gce`
ci/openshift-jenkins/extended_conformance_install	`66c9d91`	link	`/test extended_conformance_install`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

openshift-merge-robot · 2018-01-05T12:15:47Z

Automatic merge from submit-queue (batch tested with PRs 17699, 17820).

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 8, 2017

openshift-ci-robot requested review from csrwng and smarterclayton December 8, 2017 18:45

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 8, 2017

bparees force-pushed the external_buildapi2 branch from 39caa0f to 628cbdf Compare December 8, 2017 19:40

bparees commented Dec 8, 2017

View reviewed changes

bparees force-pushed the external_buildapi2 branch from bc90009 to b7f7e4f Compare December 11, 2017 18:15

gabemontero approved these changes Dec 13, 2017

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017

bparees force-pushed the external_buildapi2 branch from b7f7e4f to 74d424d Compare December 14, 2017 15:25

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2017

bparees force-pushed the external_buildapi2 branch from 74d424d to af88e7f Compare December 14, 2017 15:58

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 14, 2017

openshift-merge-robot added the vendor-update Touching vendor dir or related files label Dec 14, 2017

bparees force-pushed the external_buildapi2 branch from af88e7f to b7bada8 Compare December 14, 2017 16:31

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 14, 2017

bparees force-pushed the external_buildapi2 branch 4 times, most recently from 40b5759 to 7e64054 Compare December 15, 2017 16:36

deads2k reviewed Dec 15, 2017

View reviewed changes

bparees changed the title ~~[WIP] move builder images to use external apis~~ move builder images to use external apis Dec 19, 2017

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2017

bparees changed the title ~~move builder images to use external apis~~ [WIP] move builder images to use external apis Dec 19, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2017

bparees force-pushed the external_buildapi2 branch from 7e64054 to 3ce3e8a Compare December 19, 2017 05:47

bparees force-pushed the external_buildapi2 branch from 3ce3e8a to 05adf6e Compare January 2, 2018 14:45

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 2, 2018

bparees changed the title ~~[WIP] move builder images to use external apis~~ move builder images to use external apis Jan 2, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 2, 2018

bparees added 2 commits January 3, 2018 10:07

clean up import verifier error

b27d4b4

move builder image code to v1 apis

66c9d91

bparees force-pushed the external_buildapi2 branch from 05adf6e to 66c9d91 Compare January 3, 2018 15:11

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 3, 2018

openshift-merge-robot removed the vendor-update Touching vendor dir or related files label Jan 3, 2018

bparees added the lgtm Indicates that a PR is ready to be merged. label Jan 3, 2018

openshift-merge-robot merged commit ceb0625 into openshift:master Jan 5, 2018

bparees deleted the external_buildapi2 branch January 8, 2018 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

move builder images to use external apis #17699

move builder images to use external apis #17699

bparees commented Dec 8, 2017

bparees Dec 8, 2017

deads2k Dec 15, 2017

bparees commented Dec 11, 2017

bparees commented Dec 11, 2017

gabemontero left a comment

bparees commented Dec 13, 2017

deads2k Dec 15, 2017

bparees Dec 15, 2017

deads2k Dec 15, 2017

bparees Dec 15, 2017

deads2k Dec 19, 2017

bparees Dec 20, 2017

deads2k Dec 15, 2017

bparees Dec 15, 2017

deads2k Dec 15, 2017

bparees Dec 15, 2017

deads2k Dec 19, 2017

deads2k commented Dec 15, 2017

bparees commented Dec 19, 2017

bparees commented Dec 20, 2017

openshift-ci-robot commented Jan 3, 2018

bparees commented Jan 3, 2018

bparees commented Jan 4, 2018

openshift-bot commented Jan 5, 2018

openshift-merge-robot commented Jan 5, 2018

openshift-ci-robot commented Jan 5, 2018

openshift-merge-robot commented Jan 5, 2018

move builder images to use external apis #17699

move builder images to use external apis #17699

Conversation

bparees commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Dec 11, 2017

bparees commented Dec 11, 2017

gabemontero left a comment

Choose a reason for hiding this comment

bparees commented Dec 13, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Dec 15, 2017

bparees commented Dec 19, 2017

bparees commented Dec 20, 2017

openshift-ci-robot commented Jan 3, 2018

bparees commented Jan 3, 2018

bparees commented Jan 4, 2018

openshift-bot commented Jan 5, 2018

openshift-merge-robot commented Jan 5, 2018

openshift-ci-robot commented Jan 5, 2018

openshift-merge-robot commented Jan 5, 2018