Skip to content

remove experimental extended builds feature#16811

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
bparees:remove_extended_builds
Oct 13, 2017
Merged

remove experimental extended builds feature#16811
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
bparees:remove_extended_builds

Conversation

@bparees
Copy link
Contributor

@bparees bparees commented Oct 11, 2017

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 11, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Oct 11, 2017
@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

@smarterclayton ptal/confirm this is what you wanted. i'll send a note out to the appropriate mailing lists once we merge this. (and update docs of course).

@openshift/devex fyi

@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/unassign @mfojtik
/assign @smarterclayton

@smarterclayton
Copy link
Contributor

smarterclayton commented Oct 11, 2017

Approved

@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/assign @csrwng

@bparees bparees force-pushed the remove_extended_builds branch from bd08479 to 942afd0 Compare October 11, 2017 19:11
@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/retest

@bparees bparees force-pushed the remove_extended_builds branch from 942afd0 to 017bd6d Compare October 11, 2017 21:09
@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/retest

@bparees
Copy link
Contributor Author

bparees commented Oct 12, 2017

tests seem borked. let's see if closing/reopening helps.

@bparees bparees closed this Oct 12, 2017
@bparees bparees reopened this Oct 12, 2017
@bparees
Copy link
Contributor Author

bparees commented Oct 12, 2017

/test all

@bparees
Copy link
Contributor Author

bparees commented Oct 12, 2017

/test verify

@bparees bparees force-pushed the remove_extended_builds branch from 017bd6d to f358568 Compare October 12, 2017 01:50
@csrwng
Copy link
Contributor

csrwng commented Oct 12, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Oct 12, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, csrwng

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

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

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

@bparees
Copy link
Contributor Author

bparees commented Oct 12, 2017

/retest

jim-minter
jim-minter reviewed Oct 12, 2017
View reviewed changes
pkg/build/apis/build/v1/types.go
// empty or equal to ".", in this case it just refers to the root of WORKDIR.
// Deprecated: This feature will be removed in a future release. Use ImageSource
// to copy binary artifacts created from one build into a separate runtime image.
RuntimeArtifacts []ImageSourcePath `json:"runtimeArtifacts,omitempty" protobuf:"bytes,8,rep,name=runtimeArtifacts"`
Copy link
Contributor

@jim-minter jim-minter Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smarterclayton @bparees if we remove fields 7 and 8 from the protobuf spec, what happens if we add new fields in this struct in the future and reuse those numbers?

Copy link
Contributor Author

@bparees bparees Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed on IRC, will add a comment warning people not to reuse 7+8 until we get an answer on this from @smarterclayton or maybe @liggitt knows.

Copy link
Contributor

@liggitt liggitt Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you cannot reuse protobuf tags. add a tombstone comment like kubernetes/kubernetes@d45a342#diff-c5f41a1fd7d92b2773d2d5194a0fb6ffR2633

Copy link
Contributor Author

@bparees bparees Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @liggitt, done.

Copy link
Contributor

@liggitt liggitt Oct 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same goes for json fields, actually

@jim-minter
Copy link
Contributor

jim-minter commented Oct 12, 2017

@bparees do we get to pull config.Runtime image out of source-to-image as well?

@bparees
Copy link
Contributor Author

bparees commented Oct 12, 2017

@bparees do we get to pull config.Runtime image out of source-to-image as well?

unfortunately not. we did not treat extended builds as "experimental" in the s2i CLI tool. We can deprecate it and plan to remove it someday, but we can't just yank it.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 12, 2017
@bparees bparees force-pushed the remove_extended_builds branch from f358568 to 5e2adb3 Compare October 12, 2017 17:26
@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 12, 2017
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@bparees bparees force-pushed the remove_extended_builds branch from 5e2adb3 to f11ad0f Compare October 12, 2017 18:48
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@bparees bparees added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 12, 2017
@bparees bparees force-pushed the remove_extended_builds branch from f11ad0f to 1b95815 Compare October 12, 2017 19:24
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@bparees bparees force-pushed the remove_extended_builds branch from 1b95815 to 570ddef Compare October 12, 2017 19:31
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@bparees bparees force-pushed the remove_extended_builds branch from 570ddef to 999dc3a Compare October 12, 2017 20:23
@bparees bparees added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@bparees
Copy link
Contributor Author

bparees commented Oct 13, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

openshift-merge-robot commented Oct 13, 2017

Automatic merge from submit-queue (batch tested with PRs 16777, 16811, 16823, 16808, 16833).

@openshift-merge-robot openshift-merge-robot merged commit 2cf9082 into openshift:master Oct 13, 2017
@bparees bparees deleted the remove_extended_builds branch October 14, 2017 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@liggitt liggitt liggitt left review comments

@jim-minter jim-minter jim-minter left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@liggitt liggitt

@smarterclayton smarterclayton

@csrwng csrwng

Labels

api-approved approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@bparees @smarterclayton @csrwng @openshift-merge-robot @jim-minter @liggitt @mfojtik @openshift-ci-robot