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

Add grep_pipe #130

Merged
merged 1 commit into from
Jun 19, 2024
Merged

Add grep_pipe #130

merged 1 commit into from
Jun 19, 2024

Conversation

gbladislau
Copy link

This adds a new functionality to the ops_linux.py enabling greping on direct commands outputs.

"""
Invoke ``grep`` remotely searching for an expression in a command output.

:param session: vm session
Copy link
Contributor

Choose a reason for hiding this comment

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

:param session: session to run the command on would be more consistent


:param session: vm session
:type session: ShellSession
:param str cmd: command that its output will be grepped
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not native but this sounds strange. How about command whose output will be grepped?

Copy link
Author

Choose a reason for hiding this comment

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

Much better, I'm not native either you can see that.

:param session: vm session
:type session: ShellSession
:param str cmd: command that its output will be grepped
:param str pattern: pattern to grep
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please unify this with the grep param above? (expr rather than pattern)

:param str cmd: command that its output will be grepped
:param str pattern: pattern to grep
:param str flags: extra flags passed to ``grep`` on the command line
:param bool check: whether to quietly run grep for a boolean check
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, it'd be nice to unify this with the existing grep by changing the position of check and flags.

Something like grep_pipe(session, cmd, expr, check, flags) would be IMO better (or even session, expr, cmd, check, flags but logically the cmd should be first here. Not a strong preference over those two, though.)

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Hello @gbladislau, glad to see a new face in aexpect. Is this a theoretical improvement or are you using this somewhere? I'm not sure how important this is since one can simply use session.cmd_status("cmd | grep ...") manually but if there is a project using this frequently, we can add this as long as it's consistent with the existing grep command (arguments order and docstrings).

@gbladislau
Copy link
Author

Hi @ldoktor I just saw your review. I use the avocado project on a daily basis at the company I work for. This is just a addition to the project since I used session.cmd_status("cmd| grep ...") a lot I think it would be good to have a grep for commands. I'll make the changes from your review and push them.

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. Could you please squash it into a single commit so we can get this in?

This adds a new functionality to the ops_linux.py enabling greping
on direct commands outputs.
@gbladislau
Copy link
Author

Done!

Copy link
Contributor

@ldoktor ldoktor left a comment

Choose a reason for hiding this comment

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

thanks, lgtm

@ldoktor ldoktor merged commit 4e5ac96 into avocado-framework:main Jun 19, 2024
3 checks passed
@gbladislau gbladislau deleted the add-grep-pipe branch June 19, 2024 15:56
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.

2 participants