-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add ability to parse presanitized text #24
base: master
Are you sure you want to change the base?
Conversation
} | ||
|
||
async function runTest (dataName: string, t: any) { | ||
const actual = await readActual(dataName); | ||
const actual = await readActual(dataName, false); |
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.
Small improvement, but how would feel about simply adding the clean: boolean
parameter to runTest
and pass it through as needed. That would avoid the need for a separate runCleanTest
function which duplicates most of its logic.
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.
That sounds good to me, I actually debated doing it that way when I started looking at testing. I will give the other reviewers a chance to take a peak and then update my change based on all the feedback. 😄
Thanks for the contribution @scgodbold! Overall, this looks great to me and your use-case sounds like a valid case to me. Just a minor comment on the test helpers. I'll add a couple of other reviewers so we can get this merged soon. |
Rebased commit to upstream since changes had occured, and addressed issues in previous commit.
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.
Looks good to me @philidem you have thought about this code more than I have. Want to merge it?
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.
Instead of a clean: boolean
argument, please use a options?: ParseOptions
argument.
src/index.ts
Outdated
@@ -347,7 +348,7 @@ function parseAttributeLine (line: string, lastChange: Changed, errors: Array<Pa | |||
lastChange.changedAttributes[name] = result; | |||
} | |||
|
|||
export function parseStdout (logOutput: string): ParseResult { | |||
export function parseStdout (logOutput: string, clean: boolean): ParseResult { |
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 you change this to an options
argument that has a clean
property? I'm worried that additional options will be added in the future and it will be hard to prevent breaking change if we don't provide this flexibility now.
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.
Also, make the second options
argument optional. For example, something like:
function parseStdout (logOutput: string, options?: ParseOptions): ParseResult {
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.
Sorry for the delay, I was off on a vacation I will get going on making these changes 😄
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.
Should all be fixed
This is in reference to #23 I opened earlier this week. I had some free time and figured I would take a stab at it. It attempts to add the ability to parse pre-sanitized input such as what
terraform show
provides. This helps enable use in automation as it is best to store your plan objects to assure a consistent apply occurs.The idea being that passing a
--clean
flag to the parser tells it that only resource changes will be listed. Doing this there is no need to substring the plan to extract the changing resources.