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

fix(bootstrap): fix import command handling of no input piped to stdin #288

Merged
merged 1 commit into from
May 8, 2023

Conversation

suchapalaver
Copy link
Contributor

@suchapalaver suchapalaver commented May 4, 2023

Improve information around stdin not being readable, quit if if it's not and standard in is not a tty.
CHRON-323

@suchapalaver suchapalaver marked this pull request as ready for review May 4, 2023 16:16
@suchapalaver suchapalaver requested a review from a team as a code owner May 4, 2023 16:16
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 0a8aa20 to 1259ca1 Compare May 4, 2023 19:37
@vercel vercel bot temporarily deployed to Preview May 4, 2023 19:37 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 1259ca1 to bdc85e3 Compare May 4, 2023 19:47
@vercel vercel bot temporarily deployed to Preview May 4, 2023 19:47 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from bdc85e3 to 02ccf87 Compare May 4, 2023 21:40
@vercel vercel bot temporarily deployed to Preview May 4, 2023 21:40 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 02ccf87 to 280eda0 Compare May 4, 2023 23:31
@vercel vercel bot temporarily deployed to Preview May 4, 2023 23:32 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 280eda0 to 65be559 Compare May 5, 2023 10:46
@vercel vercel bot temporarily deployed to Preview May 5, 2023 10:46 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 65be559 to 86d0c33 Compare May 5, 2023 11:01
@vercel vercel bot temporarily deployed to Preview May 5, 2023 11:01 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 86d0c33 to c295e5d Compare May 5, 2023 11:03
@vercel vercel bot temporarily deployed to Preview May 5, 2023 11:03 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from c295e5d to 389acd1 Compare May 5, 2023 11:40
@vercel vercel bot temporarily deployed to Preview May 5, 2023 11:41 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 389acd1 to e218c8d Compare May 5, 2023 16:28
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:29 Inactive
@suchapalaver suchapalaver changed the title fix(bootstrap): fix import command hanging if no input piped to stdin fix(bootstrap): fix import command handling of no input piped to stdin May 5, 2023
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from e218c8d to d82b2eb Compare May 5, 2023 16:33
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:34 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from d82b2eb to 21d9e09 Compare May 5, 2023 16:34
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:35 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 21d9e09 to 6fcfee6 Compare May 5, 2023 16:45
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:46 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 6fcfee6 to fd9950f Compare May 5, 2023 16:54
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:54 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch 2 times, most recently from 09aedf4 to 68d174b Compare May 5, 2023 16:56
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:57 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 68d174b to 9fa2ad9 Compare May 5, 2023 16:57
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:57 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 9fa2ad9 to 56dc681 Compare May 5, 2023 16:59
@vercel vercel bot temporarily deployed to Preview May 5, 2023 16:59 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 56dc681 to 85dbc14 Compare May 5, 2023 17:18
@vercel vercel bot temporarily deployed to Preview May 5, 2023 17:18 Inactive
@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from 85dbc14 to f56765f Compare May 5, 2023 17:20
@vercel vercel bot temporarily deployed to Preview May 5, 2023 17:21 Inactive
@suchapalaver suchapalaver requested a review from mtbc May 5, 2023 17:22
@mtbc
Copy link
Contributor

mtbc commented May 6, 2023

I was thinking of something more like,

let data = if let Some(path) = matches.value_of("path") {
    let path = PathBuf::from(path);
    let data = std::fs::read_to_string(&path)?;
    info!("Loaded import data from {:?}", path);
    data
} else {
    if atty::is(atty::Stream::Stdin) {
      eprintln!("Attempting to import data from standard input, press Ctrl-D to finish.");
    }
    info!("Attempting to read import data from stdin...");
    let data = read_import_data()?;
    info!("Loaded {} bytes of import data from stdin", data.len());
    data
};

so, for instance, ... import myns my-ns-uuid gets the message about Ctrl-D, but ... import myns my-ns-uuid <from-my-data.json does not get the message printed even though, in terms of CLAP arguments, they're the same.

@suchapalaver
Copy link
Contributor Author

suchapalaver commented May 6, 2023

I was thinking of something more like,

let data = if let Some(path) = matches.value_of("path") {
    let path = PathBuf::from(path);
    let data = std::fs::read_to_string(&path)?;
    info!("Loaded import data from {:?}", path);
    data
} else {
    if atty::is(atty::Stream::Stdin) {
      eprintln!("Attempting to import data from standard input, press Ctrl-D to finish.");
    }
    info!("Attempting to read import data from stdin...");
    let data = read_import_data()?;
    info!("Loaded {} bytes of import data from stdin", data.len());
    data
};

so, for instance, ... import myns my-ns-uuid gets the message about Ctrl-D, but ... import myns my-ns-uuid <from-my-data.json does not get the message printed even though, in terms of CLAP arguments, they're the same.

Could you give what you're suggesting a try? I think you and atty might have differing definitions of what a tty is. The inputs you're giving as examples don't behave the way you're expecting - branch here.

@mtbc
Copy link
Contributor

mtbc commented May 6, 2023

I did try my suggestion before commenting. Have just done so again, still behaves exactly as I'd expect, so does your branch. If I give a filename or pipe something in, like from a file, I don't get the control+D message. If I leave it just attached to the terminal expecting input from that, I do get the message. Is that not what happens for you?

Maybe we can screenshare on Monday so I can see what you're seeing.

@suchapalaver
Copy link
Contributor Author

I did try my suggestion before commenting. Have just done so again, still behaves exactly as I'd expect, so does your branch. If I give a filename or pipe something in, like from a file, I don't get the control+D message. If I leave it just attached to the terminal expecting input from that, I do get the message. Is that not what happens for you?

Maybe we can screenshare on Monday so I can see what you're seeing.

Could you share the exact command you used to get the Ctrl + D message path, please?

@mtbc
Copy link
Contributor

mtbc commented May 6, 2023

mtbc@pop-os:~/src/chronicle$ git checkout origin/fix/what-is-a-tty
HEAD is now at 44a2049 fix(bootstrap): fix import command hanging if no input piped to stdin
mtbc@pop-os:~/src/chronicle$ git status
HEAD detached from origin/main
nothing to commit, working tree clean
mtbc@pop-os:~/src/chronicle$ cargo run --bin chronicle --features inmem -- import testns 60976119-adc6-3826-a57b-3ccb0580e7bd 
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target/debug/chronicle import testns 60976119-adc6-3826-a57b-3ccb0580e7bd`
Attempting to import data from standard input, press Ctrl-D to finish.

@suchapalaver suchapalaver force-pushed the fix/fix-import-hanging-when-no-stdin branch from f56765f to 1336d0d Compare May 6, 2023 13:36
@vercel vercel bot temporarily deployed to Preview May 6, 2023 13:36 Inactive
@suchapalaver
Copy link
Contributor Author

I did try my suggestion before commenting. Have just done so again, still behaves exactly as I'd expect, so does your branch. If I give a filename or pipe something in, like from a file, I don't get the control+D message. If I leave it just attached to the terminal expecting input from that, I do get the message. Is that not what happens for you?

Maybe we can screenshare on Monday so I can see what you're seeing.

Sorry! You were right! Don't know why I wasn't seeing it before. I made all your suggested changes.

@suchapalaver suchapalaver disabled auto-merge May 7, 2023 17:41
@suchapalaver suchapalaver merged commit b06b407 into main May 8, 2023
@suchapalaver suchapalaver deleted the fix/fix-import-hanging-when-no-stdin branch May 8, 2023 13:11
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