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

Pretty print CNI config when rendering NetAttachDefs #406

Merged
merged 2 commits into from
Mar 23, 2023

Conversation

tariq1890
Copy link
Contributor

When debugging CNI configs, it becomes a lot easier when we can see pretty-printed JSON in NetworkAttachmentDefinitions

@github-actions
Copy link

github-actions bot commented Feb 8, 2023

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

coveralls commented Feb 8, 2023

Pull Request Test Coverage Report for Build 4433914725

  • 8 of 16 (50.0%) changed or added relevant lines in 3 files are covered.
  • 6 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.2%) to 25.45%

Changes Missing Coverage Covered Lines Changed/Added Lines %
controllers/helper.go 4 6 66.67%
controllers/sriovibnetwork_controller.go 2 5 40.0%
controllers/sriovnetwork_controller.go 2 5 40.0%
Files with Coverage Reduction New Missed Lines %
controllers/sriovnetwork_controller.go 2 65.71%
test/util/util.go 4 39.73%
Totals Coverage Status
Change from base Build 4418252967: -0.2%
Covered Lines: 1921
Relevant Lines: 7548

💛 - Coveralls

@tariq1890
Copy link
Contributor Author

@adrianchiris @SchSeba @e0ne

Request review on this as well

api/v1/helper.go Outdated Show resolved Hide resolved
@tariq1890
Copy link
Contributor Author

@adrianchiris Any update on this?

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@tariq1890 tariq1890 requested a review from adrianchiris March 10, 2023 20:34
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@tariq1890
Copy link
Contributor Author

@adrianchiris I have updated the PR to ensure the rendered CNI config in the NetAttachDef is pretty-printed. Requesting your review on this

@adrianchiris
Copy link
Collaborator

/test-all

controllers/sriovibnetwork_controller.go Outdated Show resolved Hide resolved
controllers/sriovnetwork_controller.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@tariq1890 tariq1890 requested a review from adrianchiris March 13, 2023 03:07
@tariq1890
Copy link
Contributor Author

/test-all

test/util/util.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@tariq1890 tariq1890 requested review from adrianchiris and removed request for adrianchiris March 13, 2023 17:29
@tariq1890 tariq1890 requested review from adrianchiris and zeeke and removed request for adrianchiris March 13, 2023 17:29
Copy link
Member

@zeeke zeeke left a comment

Choose a reason for hiding this comment

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

/lgtm

@tariq1890 thanks for working on this

waiting for @adrianchiris approval as the code changed a bit since his last review

controllers/helper.go Outdated Show resolved Hide resolved
@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@github-actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@tariq1890
Copy link
Contributor Author

@adrianchiris I have split the commits into two. Please re-review

@tariq1890 tariq1890 requested review from adrianchiris and zeeke and removed request for adrianchiris and zeeke March 16, 2023 05:33
@adrianchiris
Copy link
Collaborator

adrianchiris commented Mar 16, 2023

lets get inputs from @SchSeba @fedepaol regarding the moved methods, i dont remember if these are used in a CI/testing proj in Openshift.

other than that im LGTM

@zeeke
Copy link
Member

zeeke commented Mar 16, 2023

I can confirm that methods GenerateExpectedNetConfig and GenerateExpectedIBNetConfig are not used by Openshift testing project:

I asked @tariq1890 to move them because they can panic since this PR, so it's better to break compile compatibility (if any) than getting a panic inside a library.

@tariq1890
Copy link
Contributor Author

Hello :) how can we move this PR forward? It's been open for a while and I've been through multiple iterations already. Hoping to close this out

@adrianchiris
Copy link
Collaborator

/test-all

@adrianchiris
Copy link
Collaborator

thx for confirming @zeeke. ill merge this one once CI pass.

@adrianchiris adrianchiris merged commit 1e0d8ab into k8snetworkplumbingwg:master Mar 23, 2023
@tariq1890 tariq1890 deleted the pretty-cni-config branch March 23, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants