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

Podman inspect completion #19261

Merged

Conversation

cgiradkar
Copy link
Contributor

Add tab-completion feature for podman inspect -f
The command:
podman inspect <ContainerID> -f "{{."
was not giving any output in console for bash-completion but the same was working fine for the command:
podman container inspect <ContainerID> -f "{{."
So added a flow to handle the former command and additionally handled all types of entities namely container, image, volume, pod and network.
This would ESSENTIALLY make the sub-commands such as follows OBSOLETE:
podman container inspect
podman image inspect
podman volume inspect
podman pod inspect
podman network inspect

Does this PR introduce a user-facing change?

The following command is now enabled with tab-completion:
podman inspect {CONTAINER | IMAGE | VOLUME | POD | NETWORK} -f "{{." <tab>
where CONTAINER can be ContainerID or ContainerName and same for other given entities.

Closes #18672

Signed-off-by: Chetan Giradkar [email protected]

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jul 17, 2023
@rhatdan
Copy link
Member

rhatdan commented Jul 17, 2023

Obviosly this does not handle the case where there are duplicate names, but we already have the problem.
LGTM
@Luap99 PTAL

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

The actual code LGTM, can you please squash the commits into one. We try to keep a clean git history and unless there is a good reason to use several commits, i.e. when making several distinct changes, we prefer one commit per feature including the tests.

run

git rebase -i HEAD~4

then select change pick to s for the last three commits.

Also I recommend to reword the commit messsage. The title should be shorter, I suggest something like: add completion for podman inspect --format. Note this go code is used by all shell completion scripts so focusing on bash is not correct. This will make it work for all supported shells.
You can change the message of the latest commit with: git commit --amend

If you need any help with these git commands let me know.

test/system/600-completion.bats Outdated Show resolved Hide resolved
test/system/600-completion.bats Outdated Show resolved Hide resolved
Comment on lines 85 to 87
- The `jq` tool is needed for parsing JSON output.
- The `bats` tool is needed for running tests.
- The `skopeo` tool is needed for fetching remote images.
Copy link
Member

Choose a reason for hiding this comment

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

There are more requirements, https://github.com/containers/podman/blob/c6f8de8a457800dfa3fb1fe7047f05f2fed34ebc/rpm/podman.spec#L142-L154C16

And if I learned one thing we are very bad at keeping these up to date. I think it would make more sense to just say the latest requirements are listed in the podman-tests rpm. We can add this command: dnf repoquery --requires podman-tests, this way we do not have to update the list here
cc @edsantiago

Copy link
Member

Choose a reason for hiding this comment

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

I think what @Luap99 meant here was that you add something like "see rpm/podman.spec, under 'package test', for a list of requirements". Duplicating the list here is unmaintainable. Too late, though; we'll have to deal with that another day.

Nice work otherwise!

@cgiradkar cgiradkar force-pushed the podman_inspect_completion branch 3 times, most recently from 3088c61 to 35a5d5b Compare July 18, 2023 12:11
@cgiradkar cgiradkar force-pushed the podman_inspect_completion branch from 35a5d5b to f9340ec Compare July 18, 2023 13:21
@Luap99
Copy link
Member

Luap99 commented Jul 18, 2023

You also have to add:

diff --git a/test/system/610-format.bats b/test/system/610-format.bats
index 3ee95bdff..d22f3924b 100644
--- a/test/system/610-format.bats
+++ b/test/system/610-format.bats
@@ -25,6 +25,7 @@ history           | $IMAGE
 image history     | $IMAGE
 image inspect     | $IMAGE
 container inspect | mycontainer
+inspect           | mycontainer
 
 volume inspect    | -a
 secret inspect    | mysecret

This fixes the system tests because we know complete go template for podman inspect.

@cgiradkar cgiradkar force-pushed the podman_inspect_completion branch from f9340ec to 00a5b07 Compare July 18, 2023 13:32
@cgiradkar cgiradkar marked this pull request as ready for review July 19, 2023 12:11
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 19, 2023
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM
@containers/podman-maintainers PTAL

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

/lgtm
Nice work!

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 19, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgiradkar, Luap99, vrothberg

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-merge-robot openshift-merge-robot merged commit 9962318 into containers:main Jul 19, 2023
@cgiradkar cgiradkar deleted the podman_inspect_completion branch August 1, 2023 18:28
@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 Oct 31, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 31, 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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Container completions broken when inspecting without specifying container in the command
6 participants