-
Notifications
You must be signed in to change notification settings - Fork 0
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 option to run with notify fifo #51
Conversation
be9d035
to
fcb3e70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. This looks much cleaner now that it's built on a base which has some infrastructure in place to accommodate different modes of operation compared to my PoC.
// Yes, this can actually be parsed as a CSV file with spaces as separators and it handles quoted string the same way a shell does. | ||
r := csv.NewReader(h.pipe) | ||
r.Comma = ' ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very nice 😄
lines, err := r.ReadAll() | ||
if err != nil { | ||
logrus.Errorf("Failed to read from fifo: %s", err) | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that's fine since we wait for the next update to try again. So no log spamming/looping should occur.
Summary
Checklist
bug
,enhancement
,documentation
,change
,breaking
,dependency
as they show up in the changelog