Skip to content

Make the start command print the endpoint on stdout #1271

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

Merged
merged 4 commits into from
Jan 13, 2021
Merged

Conversation

tobim
Copy link
Member

@tobim tobim commented Jan 8, 2021

📔 Description

This is helpful for test scripts that pass in :0 to get a random free port allocated from the operating system.

📝 Checklist

  • All user-facing changes have changelog entries.

@tobim tobim requested a review from a team January 8, 2021 15:53
@dominiklohmann
Copy link
Member

This needs a changelog entry, and it also needs the start command documentation to be updated accordingly.

Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

This feels like a not-so-nice hack. Let's try to find an alternative, or at least make it conditional on a given flag that we have to pass.

@tobim
Copy link
Member Author

tobim commented Jan 12, 2021

This feels like a not-so-nice hack. Let's try to find an alternative, or at least make it conditional on a given flag that we have to pass.

This allow us to write:

coproc nodefd {
  vast --verbosity=quiet -e :0 start
}
read -u "${nodefd[0]}" endpoint
# node is listening now
vast -e $endpoint status
...
vast -e $endpoint stop

To me this seems like very reasonable behavior and definitely not a hack. Note that reading from stderr instead is not that easy, because the buffer would eventually fill up and block the logging thread of the node in a write operation.

@mavam
Copy link
Member

mavam commented Jan 12, 2021

I understand where you are coming from, but I disagree that this warrants unconditional printing to stdout. Does anything speak against introducing an option that triggers this behavior, e.g., --print-bound-port? It's not a common case but as big consequences for launch tools that process stdout.

Also, how does your patch interact with log level "quiet"?

@dominiklohmann
Copy link
Member

This feels like a not-so-nice hack. Let's try to find an alternative, or at least make it conditional on a given flag that we have to pass.

To me, this feels like the proper way to solve this problem. It's the natural way to solve it for improved scriptability.

However, I also agree that this should be behind a flag, e.g., vast.start.show-port, and that flag should default to false.

Also, how does your patch interact with log level "quiet"?

It shouldn't have anything to do with log levels, this is printing to stdout so it can be processed by the caller. It necessarily has to be on a separate channels from regular log output.

Copy link
Member

@mavam mavam left a comment

Choose a reason for hiding this comment

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

Looks good, just a few word smithing tweaks.

@tobim tobim force-pushed the topic/print-endpoint branch from 6921755 to 00536d1 Compare January 13, 2021 20:55
@tobim tobim merged commit 5dcdf33 into master Jan 13, 2021
@tobim tobim deleted the topic/print-endpoint branch January 13, 2021 21:32
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.

3 participants