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

cmd: Forward stdin. #99

Merged
merged 1 commit into from
Jul 14, 2021
Merged

Conversation

shlevy
Copy link
Contributor

@shlevy shlevy commented Jun 30, 2021

Fixes #95.

@shlevy shlevy requested a review from a team June 30, 2021 10:05
shlevy added a commit to scarf-sh/gha-buildevents that referenced this pull request Jun 30, 2021
@MikeGoldsmith
Copy link
Contributor

Hi @shlevy thank you for your contribution.

I don't see a problem with this change, but would like to get @maplebed's opinion as they are more familiar with buildevents and to to check it would not have unexpected consequences.

Copy link
Contributor

@maplebed maplebed left a comment

Choose a reason for hiding this comment

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

Thanks for the bump, @MikeGoldsmith. Under most conditions I can think of where buildevents is used there's nothing on stdin, so this will be a noop change for those cases.

I would love to see a real world example of how this change will be used - @shlevy maybe you could drop a few examples in either #95 or this PR? I don't think we should block on waiting for examples, but I do think it will help give context to the change if it does cause problems down the line and will ensure any future fixes take into account the intent of the change.

@shlevy
Copy link
Contributor Author

shlevy commented Jul 13, 2021

Here's an example from a diff where I added instrumentation to our CI:

- kubectl exec -i svc/scarf-redis -- redis-cli < ./redis_dummydata.txt
+ buildevents cmd --quiet $TRACE_ID $STEP_SPAN_ID  load-redis-data -- kubectl exec -i svc/scarf-redis -- redis-cli < ./redis_dummydata.txt

@MikeGoldsmith MikeGoldsmith added status: review needed Changes need review. type: maintenance The necessary chores to keep the dust off. version: bump minor A PR that adds behavior, but is backwards-compatible. labels Jul 13, 2021
@MikeGoldsmith MikeGoldsmith merged commit 5382112 into honeycombio:main Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: review needed Changes need review. type: maintenance The necessary chores to keep the dust off. version: bump minor A PR that adds behavior, but is backwards-compatible.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

buildevents cmd: Forward stdin
3 participants