Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove experimental extended builds feature #16811

Merged
merged 1 commit into from
Oct 13, 2017
Merged

remove experimental extended builds feature #16811

merged 1 commit into from
Oct 13, 2017

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

Approved

@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/assign @csrwng

@bparees
Copy link
Contributor Author

bparees commented Oct 11, 2017

/retest

@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

@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

[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.

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

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

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

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

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

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

@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
@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
@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 added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017
@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 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

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

@liggitt liggitt liggitt left review comments

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

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.

None yet
8 participants
@bparees @smarterclayton @csrwng @openshift-merge-robot @jim-minter @liggitt @mfojtik @openshift-ci-robot