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: parse error when only outputs is changed #896

Merged
merged 5 commits into from
Sep 9, 2023
Merged

fix: parse error when only outputs is changed #896

merged 5 commits into from
Sep 9, 2023

Conversation

taro-kayo
Copy link
Contributor

Close #117

Related PR: mercari/tfnotify#109

if startChangeOutput != -1 && endChangeOutput == -1 && strings.HasPrefix(line, "Plan: ") { // https://github.com/hashicorp/terraform/blob/dfc12a6a9e1cff323829026d51873c1b80200757/internal/command/views/plan.go#L306
endChangeOutput = i + 1
if startChangeOutput != -1 && endChangeOutput == -1 && strings.HasPrefix(line, "-----") { // https://github.com/hashicorp/terraform/blob/1ac7a37d00f3c796f816070847bf02109cb9cab2/internal/command/views/operation.go#L142
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To include changes to outputs coming after Plan:, we have to wait for a horizontal rule which comes after Changes to Outputs:.

Comment on lines +204 to +207
// if we get here before finding a horizontal rule, output all remaining.
if endChangeOutput == -1 {
endChangeOutput = len(lines) - 1
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If TF_IN_AUTOMATION environment variable is set, no horizontal rules appear after Terraform will perform the following actions:. 😢

name: "plan output changes only pattern 0.12",
body: planOnlyOutputChangesSuccessResult0_12,
result: ParseResult{
Result: "Plan: 0 to add, 0 to change, 0 to destroy.",
Copy link
Contributor Author

@taro-kayo taro-kayo Sep 8, 2023

Choose a reason for hiding this comment

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

This is not ideal, but it's very old version... 🙈

@taro-kayo taro-kayo marked this pull request as ready for review September 8, 2023 10:09
@suzuki-shunsuke
Copy link
Owner

Thank you for your contribution!

@suzuki-shunsuke
Copy link
Owner

suzuki-shunsuke commented Sep 9, 2023

I'm reviewing this pr yet, but I found an issue.

$ terraform version
Terraform v1.5.7
on darwin_arm64
+ provider registry.terraform.io/hashicorp/github v5.35.0

main.tf

import {
  id = "tfaction"
  to = github_repository.tfaction
}

output "foo" {
  value = "foo"
}
$ tfcmt plan -- terraform plan

Changes to Outputs:
  + foo = "foo"

You can apply this plan to save these new output values to the Terraform
state, without changing any real infrastructure.

│ Error: Import block target does not exist

│   on main.tf line 1:
│    1: import {

│ The target for the given import block does not exist. If you wish to
│ automatically generate config for this resource, use the
│ -generate-config-out option within terraform plan. Otherwise, make sure the
│ target resource exists within your configuration. For example:

│   terraform plan -generate-config-out=generated.tf

tfcmt v4.5.0

image

This pull Request

image

At least, the output Changes to Outputs: doesn't necessarily mean the success of terraform plan.

@taro-kayo
Copy link
Contributor Author

Thanks for testing. 😃
I'll take a look.

@taro-kayo
Copy link
Contributor Author

taro-kayo commented Sep 9, 2023

OK, I tested.
So, it seems that the behavior in case of Terraform error is not this PR problem.
There may be room for improvement regarding the fact that Click me is not displayed in the case of no error, resource no changes, outputs changing. 😃

failed, resource changing, outputs changing

Both v4.5.0 and this PR version have the same result.

Screenshot 2023-09-09 at 14 32 29

failed, resource changing, outputs no changes

Same as failed, resource changing, outputs changing case above.

failed, resource no changes, outputs changing

v4.5.0

Screenshot 2023-09-09 at 14 37 00

this PR version

Screenshot 2023-09-09 at 14 37 42

no error, resource changing, outputs changing

v4.5.0

Screenshot 2023-09-09 at 14 46 30

this PR version

Screenshot 2023-09-09 at 14 46 45

no error, resource changing, outputs no changes

Both v4.5.0 and this PR version have the same result.

Screenshot 2023-09-09 at 14 42 35

no error, resource no changes, outputs changing

v4.5.0

Screenshot 2023-09-09 at 14 40 45

this PR version

Screenshot 2023-09-09 at 14 40 55

@taro-kayo
Copy link
Contributor Author

Fixed the problem I noticed in images in the comment above. 🎉

Problem: Note: ... is included in Change Result.
fixed: e40fa52

before

Screenshot 2023-09-09 at 16 17 37

after

Screenshot 2023-09-09 at 16 18 11


Problem: Change Result is not displayed when only tfstate outputs is changed.
fixed: 5d98cba

I copied the test cases from mercari/tfnotify#109, but it may be that Terraform's output specification changed at some point.

before

Screenshot 2023-09-09 at 16 19 54

after

Screenshot 2023-09-09 at 16 20 14

@suzuki-shunsuke
Copy link
Owner

suzuki-shunsuke commented Sep 9, 2023

Thank you for your update.
I think the meaning of the result Changes to Outputs isn't clear for users.

image

I'd like to make this message easier to understand.

For example, how about No resource changes, but Outputs are changed or Only Outputs are changed?

Plan Result

No resource changes, but Outputs are changed

Plan Result

Only Outputs are changed

What do you think? If you come up with better messages, please let me know.

@taro-kayo
Copy link
Contributor Author

That's what I thought too, but pretended not to see. 🙈

Since it is plan, how about changing it to Only output will be changed.?
I'll fix it if this is okay.

@suzuki-shunsuke
Copy link
Owner

Since it is plan, how about changing it to Only output will be changed.?

Oh, I see.
Looks good.

Indeed, Terraform uses will.

e.g.

Terraform will perform the following actions:

null_resource.bar will be created

Could you change to Only Outputs will be changed.?

  • I'd like to capitalize Outputs because this Outputs is not a general word but a Terraform specific feature
    • Terraform also capitalize Outputs: e.g. Changes to Outputs
  • I'd like to make Outputs plural

@suzuki-shunsuke
Copy link
Owner

Other than that it looks good.

@taro-kayo
Copy link
Contributor Author

Outputs looks better. 👍

fixed: 6fa879f

I'm not too sure about this fix. 😅

@suzuki-shunsuke
Copy link
Owner

Thank you for your great work!

@suzuki-shunsuke suzuki-shunsuke merged commit c4a74db into suzuki-shunsuke:main Sep 9, 2023
@taro-kayo taro-kayo deleted the fix/only-outputs-changed branch September 9, 2023 11:23
@suzuki-shunsuke suzuki-shunsuke added this to the v4.5.1 milestone Sep 9, 2023
@suzuki-shunsuke
Copy link
Owner

suzuki-shunsuke commented Sep 9, 2023

The prerelease version v4.5.1-1 is out.
https://github.com/suzuki-shunsuke/tfcmt/releases/tag/v4.5.1-1

We'll verify this, and if there is no problem, we'll release v4.5.1.

@suzuki-shunsuke
Copy link
Owner

v4.5.1 is out 🎉
https://github.com/suzuki-shunsuke/tfcmt/releases/tag/v4.5.1

Thank you for your contribution!

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.

parse error
2 participants