Prefer reading query from stdin if available #1917
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A recent change made it so not providing a query causes everything to be exported. However, that made it so providing a query on stdin required the option `vast.export.read` to be set to `"-"`, which was far from intuitive. This change streamlines the option, giving the user an error if conflicting ways to read the query exist and preferring stdin over exporting everything if a query is provided on stdin but not on the command line.
dominiklohmann
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
158a663 to
c5cec4f
Compare
dominiklohmann
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
2244908 to
19de763
Compare
mavam
reviewed
Oct 21, 2021
dominiklohmann
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
7695435 to
a727c20
Compare
|
I am really stumped at why the CI makes the integration test framework think we have input on stdin, when we really don't. While we could downgrade the error to a warning and simply live with it—everything works as expected locally for both @tobim and me—, I would like to understand what's wrong here first. |
dominiklohmann
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
a727c20 to
f0bacf8
Compare
lava
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
11 times, most recently
from
3221101 to
1736530
Compare
lava
reviewed
Oct 28, 2021
tobim
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
1736530 to
51cbd60
Compare
Using `!isatty()` is didn't work in case vast run in a scrip in headleass mode, i.e. during the integration tests in CI. We now use stat to check if the input is a pipe or a regular file.
tobim
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
6a7cd64 to
8002d83
Compare
lava
approved these changes
Oct 28, 2021
There was a problem hiding this comment.
Things seem to work now (and even if it turns out the new logic is still not quite what we want, this should at least not be a regression), so I think with the CI going through we're good to go.
There are a few more nits below, but I think none of them is important enough to justify delaying the rc any further.
dominiklohmann
commented
Oct 29, 2021
lava
approved these changes
Nov 1, 2021
lava
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
beb7156 to
37b07dc
Compare
lava
force-pushed
the
story/ch29085/prefer-stdin-over-exporting-all
branch
from
37b07dc to
b7cd11d
Compare
This test provides an explicit query as input, so according to the updated logic this should not be interpreted as "export everything" anymore but as a query, and the reference data was reverted to the original state accordingly.
|
Finally, what a pesky issue! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📔 Description
A recent change made it so not providing a query causes everything to be exported. However, that made it so providing a query on stdin required the option
vast.export.readto be set to"-", which was far from intuitive. This change streamlines the option, giving the user an error if conflicting ways to read the query exist and preferring stdin over exporting everything if a query is provided on stdin but not on the command line.📝 Checklist
🎯 Review Instructions
It's a simple change; reading the code and passing CI should suffice.