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(cli): parse JSON output #774

Merged
merged 2 commits into from
Jun 15, 2021
Merged

fix(cli): parse JSON output #774

merged 2 commits into from
Jun 15, 2021

Conversation

DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt commented Jun 10, 2021

Alternative approach for #770, I think I like it better. I strongly typed every possible message mentioned in https://www.terraform.io/docs/internals/machine-readable-ui.html (thank you @jsteinich for pointing me to it).

@DanielMSchmidt DanielMSchmidt force-pushed the fix-tfc-parsing-error-2 branch 3 times, most recently from 33ac141 to 4c926ea Compare June 10, 2021 18:57
@jsteinich
Copy link
Collaborator

We'll need to supply the -json argument to the Terraform cli commands. Hopefully this is just ignored in versions < 0.15.3, but if not, will need to omit.

We also probably aren't yet at a point where we can drop support for < 0.15.3, so either need to be explicit about when to parse JSON or try always and fall back to old behavior on failure.

@DanielMSchmidt DanielMSchmidt force-pushed the fix-tfc-parsing-error-2 branch 2 times, most recently from 4e8866c to 0839873 Compare June 12, 2021 19:49
@skorfmann
Copy link
Contributor

We'll need to supply the -json argument to the Terraform cli commands. Hopefully this is just ignored in versions < 0.15.3, but if not, will need to omit.

Looks like the cli fails prior to 0.15.3.

We also probably aren't yet at a point where we can drop support for < 0.15.3, so either need to be explicit about when to parse JSON or try always and fall back to old behavior on failure.

I think we should be rather aggressive here. Probably with 0.5 we should make a cut and support Terraform 1.0 only. Since that's affecting the CLI only, users still have the option to fallback to an older Terraform CLI by using it directly.

@skorfmann
Copy link
Contributor

Alternative approach for #770, I think I like it better. I strongly typed every possible message mentioned in

That's pretty cool 👍

The tests are failing due to the log statements. Is there anything else which is to do here?

@jsteinich
Copy link
Collaborator

I think we should be rather aggressive here. Probably with 0.5 we should make a cut and support Terraform 1.0 only. Since that's affecting the CLI only, users still have the option to fallback to an older Terraform CLI by using it directly.

I'm a bit torn here. I understand not wanting to have baggage, but Terraform has been moving through versions quickly (0.15 came out 2 months ago, 0.14 was 6 months ago). I personally have stacks created with cdktf that are on 0.13.x.

Module are provider versions are always compatible across Terraform versions, so this can bleed outside of just the cli.

@DanielMSchmidt
Copy link
Contributor Author

The tests are failing due to the log statements. Is there anything else which is to do here?

Mainly debug the failing tests, if it's just the logs I'll remove them now 👍

@skorfmann
Copy link
Contributor

I think we should be rather aggressive here. Probably with 0.5 we should make a cut and support Terraform 1.0 only. Since that's affecting the CLI only, users still have the option to fallback to an older Terraform CLI by using it directly.

I'm a bit torn here. I understand not wanting to have baggage, but Terraform has been moving through versions quickly (0.15 came out 2 months ago, 0.14 was 6 months ago). I personally have stacks created with cdktf that are on 0.13.x.

Is there any blocker preventing you from upgrading?

@jsteinich
Copy link
Collaborator

Is there any blocker preventing you from upgrading?

Time

@skorfmann
Copy link
Contributor

Is there any blocker preventing you from upgrading?

Time

Hehe, ok :)

Right now it works for both output versions (text / json). At latest with Terraform 1.1, we're going to drop support for 0.15 anyway. However, I'd still push for an earlier move in that direction. And again, this only affects the cdktf-cli anyway, deploy, diff and destroy particularly

@skorfmann skorfmann merged commit 33b5132 into main Jun 15, 2021
@skorfmann skorfmann deleted the fix-tfc-parsing-error-2 branch June 15, 2021 08:05
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants