Skip to content

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Aug 18, 2025

Alternative Solutions

I have also considered implementing this via iter.Seq which would make for a simpler interface that caller can just range over but that would probably imply ignoring any and all errors associated with reading and parsing individual messages, which does not seem like an ideal design.

TODO

  • Manually test with any provider known to implement lists
  • Add unit tests

@radeksimko radeksimko changed the title (Re)Implement QueryJSON as LogMsg emitter (Re)Implement QueryJSON as LogMsg emitter Aug 18, 2025
@SBGoods
Copy link

SBGoods commented Sep 2, 2025

@rainkwan and I just tested these changes along with the new tfjson changes in terraform-plugin-testing and I think we're encountering a deadlock at the wg.wait() in runTerraformCmd() because the pw writer pipe that was defined is blocked waiting on the pr reader pipe to read.

@radeksimko
Copy link
Member Author

@SBGoods I am aware and working through resolving that.

@radeksimko radeksimko force-pushed the radek/f-query-log-emitter branch from b3bfedb to ca4fa35 Compare September 3, 2025 11:41
Copy link

hashicorp-cla-app bot commented Sep 3, 2025

CLA assistant check
All committers have signed the CLA.

@radeksimko radeksimko force-pushed the radek/f-query-log-emitter branch from ca4fa35 to 9a8d530 Compare September 3, 2025 11:46
@radeksimko radeksimko force-pushed the radek/f-query-log-emitter branch 2 times, most recently from 173d41e to 4ec9059 Compare September 3, 2025 11:52
@radeksimko radeksimko force-pushed the radek/f-query-log-emitter branch from 4ec9059 to ff3f3e6 Compare September 3, 2025 12:02
@radeksimko radeksimko force-pushed the radek/f-query-log-emitter branch from ff3f3e6 to c05687b Compare September 3, 2025 14:39
Comment on lines +226 to +230
// TODO: handle error
_ = pr.Close()
// TODO: handle error
_ = pw.Close()
// TODO: handle error
Copy link
Member Author

Choose a reason for hiding this comment

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

TBD: We could surface these somehow through additional methods or fields on LogMsgEmitter though that makes them time/ordering dependent. Perhaps it's obvious that you can't possibly have such errors until you have finished reading the last message but perhaps it's not.

Also we should probably work out what these errors could possibly be and what is the likelihood of them occurring all at the same time - i.e. whether we really need 3 separate fields/methods for those or whether we can just have 1.

// Stdout reader is closed when the last message is received.
//
// Error returned can be related to decoding of the message (nil, true, error)
// or closing of stdout reader (nil, false, error).
Copy link
Member Author

Choose a reason for hiding this comment

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

This part of the comment is actually no longer true as it relates to earlier version of the code.

I'm still unsure where exactly it is best to close the stdout reader and how/whether to surface the error from that.

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