Skip to content

system connection remove: use Args function to validate#23326

Merged
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Luap99:connection-remove-args
Jul 18, 2024
Merged

system connection remove: use Args function to validate#23326
openshift-merge-bot[bot] merged 1 commit intocontainers:mainfrom
Luap99:connection-remove-args

Conversation

@Luap99
Copy link
Copy Markdown
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

@openshift-ci
Copy link
Copy Markdown
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

Details 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 mentioned this pull request Jul 18, 2024
Draft
@Luap99 Luap99 added the No New Tests Allow PR to proceed without adding regression tests label Jul 18, 2024
@edsantiago
Copy link
Copy Markdown
Member

edsantiago commented Jul 18, 2024 •
edited
Loading

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
Copy Markdown
Member

edsantiago commented Jul 18, 2024

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

@Luap99
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

edsantiago commented Jul 18, 2024

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

@Luap99
system connection remove: use Args function to validate
85f4f89 
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 <pholzing@redhat.com>
@Luap99 Luap99 force-pushed the connection-remove-args branch from 3d18d23 to 85f4f89 Compare July 18, 2024 14:40
@Luap99
Copy link
Copy Markdown
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 Luap99 mentioned this pull request Jul 18, 2024
Merged
@Luap99
Copy link
Copy Markdown
Member Author

Luap99 commented Jul 18, 2024

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

ashley-cui
ashley-cui reviewed Jul 18, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

edsantiago commented Jul 18, 2024

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
@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.

Reviewers

@ashley-cui ashley-cui ashley-cui left review comments

Assignees

@edsantiago edsantiago

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Luap99 @edsantiago @ashley-cui