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.

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

WIP Fix image build #8

Closed
wants to merge 2 commits into from
Closed

Conversation

danwinship
Copy link
Contributor

No description provided.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 15, 2019
Copy link
Contributor

@pecameron pecameron left a comment

Choose a reason for hiding this comment

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

@danwinship I got tagged to review this. Is this work in progress? there is no description.
A note on kubectl vs oc: Guru doesn't like to see OpenShift specific anything in ovn. There was a spot where I couldn't figure out the kubectl equivalent of an oc. Considering the size of kubectl/oc it would be good to find a way to eliminate it.

This reverts commit 84ccb2e.
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2019
@danwinship
Copy link
Contributor Author

Is this work in progress?

Yes, hence the "WIP" in the title. (There is unfortunately no way, AFAIK, to get openshift-ci-bot to not tag people for review.)

A note on kubectl vs oc

In OpenShift they are they same binary. But the image we're grabbing it from only has /usr/bin/oc on disk, so we copy that over, and then symlink it to kubectl as well. So this isn't introducing any oc dependency, it's just getting an OpenShift-built kubectl binary into the OpenShift-built ovn-kubernetes image.

@pecameron
Copy link
Contributor

@danwinship originally oc/kubernetes was not needed in the image and then we rearranged the install and got included. It would be great if didn't need it.

@openshift-ci-robot
Copy link
Contributor

@danwinship: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws 3691e60 link /test e2e-aws

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.

@danwinship
Copy link
Contributor Author

Currently ovnkube.sh requires kubectl. In fact, it requires kubectl in more places than it did 6 months ago. We can try to get rid of that dependency upstream, but for now, that dependency is there

@danwinship danwinship closed this May 29, 2019
@danwinship danwinship deleted the build-test branch May 29, 2019 13:57
Billy99 added a commit to Billy99/ovn-kubernetes that referenced this pull request Nov 2, 2022
Add support to run in SmartNIC environment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants