-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
switch to hyperkube and remove renames #17369
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k 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 |
wow, that worked better than it had any right to. |
3638168
to
4f6fd90
Compare
/retest |
4f6fd90
to
e05f535
Compare
test/cmd/help.sh
Outdated
os::cmd::expect_success 'openshift admin' | ||
os::cmd::expect_success 'openshift start kubernetes' | ||
os::cmd::expect_success 'kubernetes' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add test for hyperkube?
LGTM |
e05f535
to
2bce085
Compare
@@ -421,12 +415,7 @@ install -p -m 755 contrib/migration/* %{buildroot}%{_datadir}/%{name}/migration/ | |||
%doc README.md | |||
%license LICENSE | |||
%{_bindir}/openshift | |||
%{_bindir}/kube-apiserver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need symlinks here for backwards compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need symlinks here for backwards compatibility?
A symlink wouldn't work. You could create a bash script if you really wanted to. @smarterclayton didn't indicate it was a requirement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @sdodson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't have any dependencies on kube commands in the installer. This should be fine.
2bce085
to
0c5083b
Compare
0c5083b
to
57a5bef
Compare
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue. |
@deads2k: The following tests failed, say
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. |
You're now building hyperkube twice:
Need to remove it from the IMAGE specific targets now that you made it a normal target. |
Image targets? What have we built?! |
:)
Image targets are things we only build for Linux today and which are
primarily used in images. The stock cross compile target is for anything
we expect to work across all platforms for dev and test and end users.
I guess we’ll want hyperkube in cross at some point, but only when we
actually do the split.
On Nov 22, 2017, at 7:32 AM, David Eads <[email protected]> wrote:
Need to remove it from the IMAGE specific targets now that you made it a
normal target.
Image targets? What have we built?!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#17369 (comment)>,
or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABG_p-uN-4GamI25SubCpXT9xs59x-dMks5s5BRigaJpZM4Qijq8>
.
|
This publishes hyperkube and switches to use it instead of renaming the openshift binary.