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

system connection remove: use Args function to validate #23326

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jul 18, 2024

Using the ExactArgs(1) function is better because we have less duplication of the error text and the ValidArgsFunction uses that to suggest shell completion. The command before this commit would suggest connection names even if there was already one arg on the cli set.

Does this PR introduce a user-facing change?

None

Copy link
Contributor

openshift-ci bot commented Jul 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2024
@Luap99 Luap99 added the No New Tests Allow PR to proceed without adding regression tests label Jul 18, 2024
@edsantiago
Copy link
Member

edsantiago commented Jul 18, 2024

Will this PR resolve the following flake?

   [08:08:53.654983172] $ /home/esm/src/atomic/2018-02.podman/libpod/bin/podman __completeNoDesc  commit                                                        
   [08:08:54.794829256] [Debug] [Error] could not find container 1d7c59f6127f23f60b0d1aa061d76c0e6d3570d5c568c08f908e9e378a53e105 pod (id 09b5f4e71d6ac3fd532b92
b287e3e3a4e639dc6fec4e8a5637f514333915fbfc) in state: no such pod                                                                                               
   :4                                                                                                                                                           
   Completion ended with directive: ShellCompDirectiveNoFileComp                                                                                                
   #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv                                                                                                              
   #|     FAIL:  commit: actual container listed in suggestions                                                                                                 
   #| expected: '.*-c_t112-xgnokdit                                                                                                                             
   ' (using expr)                                                                                                                                               
   #|   actual: '[Debug] [Error] could not find container 1d7c59f6127f23f60b0d1aa061d76c0e6d3570d5c568c08f908e9e378a53e105 pod (id 09b5f4e71d6ac3fd532b92b287e3e
3a4e639dc6fec4e8a5637f514333915fbfc) in state: no such pod'                                                                                                     
   #|         > ':4'                                                                                                                                            
   #|         > 'Completion ended with directive: ShellCompDirectiveNoFileComp'                                                                                 
   #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(one-off just now, on my laptop, with #23325 but without #23326. And, sorry, I did not save logs. Will apply #23326 and keep trying, with logs).

@edsantiago
Copy link
Member

Test failure is real; new code needs to account for --all

@Luap99
Copy link
Member Author

Luap99 commented Jul 18, 2024

Test failure is real; new code needs to account for --all

Oh how did I miss that, of course there was a reason for it in the first place.

@Luap99
Copy link
Member Author

Luap99 commented Jul 18, 2024

(one-off just now, on my laptop, with #23225 but without #23226. And, sorry, I did not save logs. Will apply #23226 and keep trying, with logs).

Numbers are off?

@edsantiago
Copy link
Member

Ugh. Sorry, fixed. Anyhow, no, that failure is pretty persistent, I'm trying to reproduce it and will file an issue

Using the ExactArgs(1) function is better because we have less
duplication of the error text and the ValidArgsFunction uses that to
suggest shell completion. The command before this commit would suggest
connection names even if there was already one arg on the cli set.

However because there is the --all option we still must exclude that
first.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99 Luap99 force-pushed the connection-remove-args branch from 3d18d23 to 85f4f89 Compare July 18, 2024 14:40
@Luap99
Copy link
Member Author

Luap99 commented Jul 18, 2024

Nah that would still be #23282, my PR should definitely fix the no such pod error, just because I added more context doesn't mean the cause is different. Something must be weird about how we lock things there

@Luap99
Copy link
Member Author

Luap99 commented Jul 18, 2024

@containers/podman-maintainers PTAL This is needed for the parallel sys test work.

Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

LGTM

@edsantiago
Copy link
Member

LGTM and I'm not seeing the otherwise-almost-constant completion test failure. Thank you!

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 964b9a2 into containers:main Jul 18, 2024
82 checks passed
@Luap99 Luap99 deleted the connection-remove-args branch July 18, 2024 16:43
@stale-locking-app stale-locking-app 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 17, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Oct 17, 2024
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. No New Tests Allow PR to proceed without adding regression tests release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants