Skip to content

Conversation

@danwinship
Copy link
Contributor

We are currently building (most of) the SDN code on Darwin and Windows, even though it is totally linux-specific.

I ran into this when trying to make more use of the "netlink" library in the sdn code (#15834). The library only builds on linux, so any file that imports it has to also be only built on linux.

So this tries to fix the main openshift binary to only include the SDN code when building on Linux. I don't know if it's correct stylistically. I'm happy to rewrite it however.

@openshift/networking FYI

@openshift-merge-robot openshift-merge-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 17, 2017
@smarterclayton
Copy link
Contributor

Yay, build-cross got triggered automatically. So happy

@danwinship
Copy link
Contributor Author

OK, cross-build succeeded. Other failures look like flakes.

I had to tweak hack/import-restrictions.json to get "make verify" to pass, and probably that should be fixed differently. I guess one approach would be to move the SDNNode and SDNProxy interfaces into a new directory (neither pkg/sdn/plugin nor pkg/sdn/apis) so we can keep pkg/sdn/apis pure?

],
"allowedImportPackageRoots": [
"vendor/k8s.io/apimachinery",
"vendor/k8s.io/kubernetes/pkg/proxy/config",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this won't be allowed - @deads2k for his goals here

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this won't be allowed - @deads2k for his goals here

Our api packages should only be for the type definitions themselves. We're going to build a client-go library for openshift (see attempts here: openshift/client-go#2) and the dependencies for that library need to be reasonable. Our API types are required, but the interfaces which make them more useable for runtime purposes aren't generally needed by the client, so they should live a separate package.

@danwinship danwinship force-pushed the sdn-linux-only branch 3 times, most recently from 920baa7 to 9d0d7f1 Compare August 18, 2017 16:29
@danwinship
Copy link
Contributor Author

/retest
after revert of bad commit to master

@danwinship
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 18, 2017
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2017
@danwinship
Copy link
Contributor Author

/retest

@danwinship
Copy link
Contributor Author

flake #14385
/test extended_networking_minimal
/test extended_networking

@danwinship
Copy link
Contributor Author

All tests pass now
@smarterclayton ok now? I moved the OpenShift-internal SDN interfaces to the top level of pkg/sdn to get rid of the extra imports in pkg/sdn/apis/network

@deads2k
Copy link
Contributor

deads2k commented Aug 24, 2017

Glad to see the move to start slimming down the package

/approve

lgtm if it's basically a straight move

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2017
@deads2k
Copy link
Contributor

deads2k commented Aug 24, 2017

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, deads2k

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

@openshift-merge-robot
Copy link
Contributor

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

@danwinship
Copy link
Contributor Author

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15904, 15962, 15838, 15965, 15963)

@openshift-merge-robot openshift-merge-robot merged commit be52b45 into openshift:master Aug 26, 2017
@danwinship danwinship deleted the sdn-linux-only branch August 26, 2017 13:47
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. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants