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

SDN-1521: oc-plumbing-related-refactoring #26

Merged

Conversation

tssurya
Copy link
Contributor

@tssurya tssurya commented Jan 15, 2021

Though the change looks big, its not:

  • I've refactored the folder structure under debug-scripts since in the image all scripts get copied into /usr/bin without retaining their folder structure. In future when we have more files, we can start using folders for arrangement.

  • I've removed the version and debug files since they don't seem really necessary.

  • I've moved some common functions into common.

  • Added the integration script for oc plumbing network-tools

  • SDN scripts were not working. Tidied them up and made them similar to OVN scripts.

TODOs after getting the client plumbing merged on the oc side:

  • There will be upcoming PRs that add a basic set of instructions for developers when adding scripts to the repo so that there is some sort of organization
  • Refactor the sdn_cluster_connectivity and sriov_connectivity scripts.

/cc @rcarrillocruz @juanluisvaladas : PTAL.
@astoycos : I've also modified your script moving the pod creation portions to a more common place. You can also push a PR later on to change the rest of the script accordingly and remove the comment of the script from the network-tools call.

Advise to reviewers: Go commit by commit than going through all the files. It will be easier to reivew.

@tssurya
Copy link
Contributor Author

tssurya commented Jan 15, 2021

/hold

until #24 merges.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 15, 2021
@tssurya tssurya force-pushed the oc-plumbing-related-refactor branch from ffa9e6b to 5fdd314 Compare January 28, 2021 20:38
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2021
@tssurya
Copy link
Contributor Author

tssurya commented Jan 28, 2021

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021
@tssurya tssurya changed the title SDN-1521: oc-plumbing-related-refactoring-ovn-scripts SDN-1521: oc-plumbing-related-refactoring Jan 28, 2021
@tssurya tssurya force-pushed the oc-plumbing-related-refactor branch from 5fdd314 to 5ba6d7f Compare January 29, 2021 11:20
@tssurya
Copy link
Contributor Author

tssurya commented Jan 29, 2021

/assign @juanluisvaladas @rcarrillocruz @astoycos

Copy link
Contributor

@juanluisvaladas juanluisvaladas left a comment

Choose a reason for hiding this comment

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

Seems pretty good, I just have a couple nitpicks.

debug-scripts/common Outdated Show resolved Hide resolved
debug-scripts/sdn_pod_to_svc_connectivity Outdated Show resolved Hide resolved
debug-scripts/sdn_node_connectivity Outdated Show resolved Hide resolved
debug-scripts/sdn_pod_to_pod_connectivity Outdated Show resolved Hide resolved
debug-scripts/sdn_pod_to_svc_connectivity Outdated Show resolved Hide resolved
debug-scripts/ovn_pod_to_pod_connectivity Outdated Show resolved Hide resolved
debug-scripts/ovn_pod_to_svc_connectivity Outdated Show resolved Hide resolved
debug-scripts/network-tools Outdated Show resolved Hide resolved
@tssurya tssurya force-pushed the oc-plumbing-related-refactor branch from 5ba6d7f to 2abf996 Compare January 29, 2021 13:12
@tssurya
Copy link
Contributor Author

tssurya commented Jan 29, 2021

Seems pretty good, I just have a couple nitpicks.

thanks Juan for the review. Have addressed the nits.

@juanluisvaladas
Copy link
Contributor

/approve
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2021
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: juanluisvaladas, tssurya

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 Jan 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 173dc85 into openshift:master Jan 29, 2021
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants