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

--format and --filter options for network ls and network inspect command #6161

Merged

Conversation

kunalkushwaha
Copy link
Collaborator

This PR is for fixing bug #5205
This adds following options to network commands

  • podman network ls --filter option
  • podman network inspect --format option

NOTE: Since the CNI conf file does not have defined structure and is read as read & stored as map of interfaces, its difficult to get similar output as of docker as specified in #5205 (comment). until we create an internal structure. Which may not be good idea, as it may require to sync up with libcni code. Ideally, this structure should be defined in libcni code.

PTAL: @medyagh @afbjorklund, If this helps your use case.

@sujil02
Copy link
Member

sujil02 commented May 11, 2020

It might be a good idea to add some test cases here https://github.com/containers/libpod/blob/master/test/e2e/network_test.go#L95 corresponding to new flags.

@TomSweeneyRedHat
Copy link
Member

Quick peruse LGTM. Need tests and also updates to ./completions/bash/podman

@rhatdan
Copy link
Member

rhatdan commented May 13, 2020

/approve
/lgtm
/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 13, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kunalkushwaha, rhatdan

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 lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 13, 2020
Copy link
Member

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

just a few nits that others may thumb wrestle with me over.
Otherwise LGTM

docs/source/markdown/podman-network-ls.1.md Outdated Show resolved Hide resolved
docs/source/markdown/podman-network-ls.1.md Outdated Show resolved Hide resolved
@sujil02
Copy link
Member

sujil02 commented May 14, 2020

@rhatdan: Need further opinion. I might be wrong.

Generally from what I have seen filters are usually applied lower on the stack bypassing the filter as params down. From how its done here filters for the network cannot be used by remote clients resulting in ambiguity in the responses. This can be considered a workaround in my opinion.

@rhatdan
Copy link
Member

rhatdan commented May 14, 2020

@sujil02 Not sure what you are saying, But I believe the filtering should be taking place on the server side not in the client.

@sujil02
Copy link
Member

sujil02 commented May 14, 2020

Yeah, like said the filtering should be done on the server-side from how it's done here is client-side so the api for network list will not be able to implement filters as it is not bonded to the url.values() and passed down. Example Like done here https://github.com/containers/libpod/blob/master/cmd/podman/pods/ps.go#L87
https://github.com/containers/libpod/blob/master/pkg/bindings/pods/pods.go#L128

@QiWang19
Copy link
Contributor

yes, I think this filter should be passing down. With the tunnel mode NetworkList hasn't been implemented yet, maybe for this PR is should pass to pkg/domain/infra/abi/network.go?

@kunalkushwaha
Copy link
Collaborator Author

Got it. I will move the filter code from cli to pkg

filter option helps to filter output based on name or supported plugins
by CNI networks.

Signed-off-by: Kunal Kushwaha <[email protected]>
This helps user to print the inspect output in go template format.

Signed-off-by: Kunal Kushwaha <[email protected]>
New testcase for network ls --filter and inspect --format
added. Also bash completion options updated.

Signed-off-by: Kunal Kushwaha <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 20, 2020
@QiWang19
Copy link
Contributor

LGTM

@rhatdan
Copy link
Member

rhatdan commented May 20, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2020
@mheon
Copy link
Member

mheon commented May 20, 2020

/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 May 20, 2020
@openshift-merge-robot openshift-merge-robot merged commit 588df90 into containers:master May 20, 2020
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants