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

Split up SDN master/node/proxy/CNI code #15781

Merged
merged 3 commits into from
Aug 30, 2017

Conversation

danwinship
Copy link
Contributor

Right now pkg/sdn/plugin is a mix of code that runs on the master and code that runs on nodes. This splits things up. I think it makes sense? @openshift/networking what do you think?

This gets rid of the pkg/sdn/plugin directory completely; node-specific code is now in pkg/sdn/node because it doesn't actually use the network plugin API at all any more.

@openshift-merge-robot openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2017
@rajatchopra
Copy link
Contributor

rajatchopra commented Aug 15, 2017

Looks clean to me. Squash the commits maybe to a recent date.

A minor point that came up in my head:
Not sure but other projects may be referencing our source code. Let us be aware of that and be sure if it may affect them. e.g. ovn-kubernetes project uses the IPAM from our utils.

@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 16, 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 16, 2017
@danwinship
Copy link
Contributor Author

/test extended_conformance_gce
flake #15801

Copy link
Contributor

@dcbw dcbw left a comment

Choose a reason for hiding this comment

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

I like this split. LGTM

@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 19, 2017
@imcsk8
Copy link
Contributor

imcsk8 commented Aug 22, 2017

LGTM

@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 24, 2017
@danwinship
Copy link
Contributor Author

/retest

Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM. This is great. Thank you.

@knobunc
Copy link
Contributor

knobunc commented Aug 25, 2017

I will say the right incantation on Monday to allow it to merge :-)

@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 26, 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 26, 2017
@danwinship
Copy link
Contributor Author

flake #15969
/retest

@knobunc
Copy link
Contributor

knobunc commented Aug 29, 2017

/lgtm

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

/lgtm

@smarterclayton
Copy link
Contributor

smarterclayton commented Aug 29, 2017 via email

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, knobunc, pravisankar, smarterclayton

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

@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 29, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@danwinship
Copy link
Contributor Author

/test extended_conformance_install_update

@dobbymoodge
Copy link
Contributor

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 15862, 15781, 15944)

@openshift-merge-robot openshift-merge-robot merged commit 558cdda into openshift:master Aug 30, 2017
@danwinship danwinship deleted the split-master-node branch August 31, 2017 11:54
openshift-merge-robot added a commit that referenced this pull request Aug 31, 2017
Automatic merge from submit-queue (batch tested with PRs 16054, 16057, 15440, 16028, 16055)

Rename pkg/sdn to pkg/network, for consistency with its API

@deads2k wanted this

This is on top of #15781, but that should be merged soon. Only the last two commits ("Rename pkg/sdn to pkg/network, for consistency with its API" and "Regenerate files") are for this PR.
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. component/networking lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet