-
Notifications
You must be signed in to change notification settings - Fork 33
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 issue #20 "unexpected end of JSON input" error #64
Conversation
This addresses #20 and ensures input is read from the plan file -- and not via STDIN -- if a plan file argument is provided. This also seeks to improve some of the error messaging to be a bit more clear.
Ideally, tf-summarize would feature a suite of automated tests verifying its functionality. In absenece of that, this demos the issue #20 fix via GH Actions.
- ensure the generation of the `example` directory data is done in a consistent, reproducible fashion - ensure GH Actions uses the same version of TF expected by the `example` directory - add plan and plan JSON files to source control, for testing purposes
👋 Hi @dineshba - Have you been able to take a look at this? I'm curious to hear your thoughts. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add tfplan in example folder to .gitignore?
Other than this, all changes good. Really appreciate the effort and the fix 👌 |
@dineshba Good call; I've removed the |
This fixes issue #20 by correcting the
reader.CreateReader
logic determining whether to use areader.FileReader
orreader.StdinReader
. Previously, in some environments (like GH Actions Ubuntu runners),tf-summarize
incorrectly attempted to use areader.StdinReader
, even when an explicit filename argument is provided.Ideally,
tf-summarize
would feature automated tests through which I could prove the bug & fix.In absence of such tests,I added ademo
GitHub Actions job provingtf-summarize
continues to work.However, I'm happy to add real tests (either in this PR, or in a followup PR), if you prefer those, @dineshba (I'm also happy to remove thedemo
GH Actions job from this PR if you prefer it not clutter.github/workflows/build.yml
).Update: I added 2 additional commits:
example
projecttf-summarize -md plan.json
andcat plan.json | tf-summarize -md
both continue to work as expected.Note that #63 shows reproduction of the bug via a similar GitHub Actions-based demo.