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

Implement the grep_file_contents action #70

Merged
merged 4 commits into from
Sep 27, 2024

Conversation

panhania
Copy link
Member

No description provided.

@panhania panhania requested a review from s-westphal September 25, 2024 15:11

import "rrg/fs.proto";

message Args {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense to add an option to return the whole line with the match, or also lines before and after, like the -A -B -C. (Might be also just something to consider for the future)

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it make sense to add an option to return the whole line with the match (...)

This can be easily achieved by rewriting the regex: instead of foo you match against ^.*foo.*$, thus seems like a redundant functionality.

(...) or also lines before and after, like the -A -B -C. (Might be also just something to consider for the future)

This is an interesting proposal that I can see being occasionally useful. I am hesitant to implement it right now as we don't have use case for it yet and it will interact in weirdly once we start enforcing line limit. There are two reasons for the limit: keeping agent memory usage in check and not going over the Fleetspeak message size limit. If we allow only up to 1 MiB of line data, we guarantee that we will not go over the Fleetspeak limit. With multiple lines this won't be the case—not impossible to make it work in non-pathological cases but it adds a lot of complexity.

But we can keep it in mind for the future.

@panhania panhania merged commit e8f8876 into google:master Sep 27, 2024
7 checks passed
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