Github diff markdown formatting does not work properly for embedded YAML content

[dyurovskykh-tivo]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request. Searching for pre-existing feature requests helps us consolidate datapoints for identical requirements into a single place, thank you!
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request.
• If you are interested in working on this issue or have submitted a pull request, please leave a comment.

---

Overview of the Issue

Github diff markdown formatter (added with #1751) does not work properly for embedded YAML content because it incorrectly treats hyphen - symbol, used to denote an array element in YAML, as diff removal.

Reproduction Steps

Example of Terraform code:

provider "aws" {
  region = "us-east-1"
}

data "template_file" "yaml_file" {
  template = <<EOT
array:
  - value1
  - value2
EOT
}

resource "aws_api_gateway_rest_api" "rest_api" {
  name        = "test"
  body        = data.template_file.yaml_file.rendered
  endpoint_configuration {
    types = ["REGIONAL"]
  }
}

GitHub comment created by Atlantis:

GitHub comment source:

  # aws_api_gateway_rest_api.rest_api will be created
+   resource "aws_api_gateway_rest_api" "rest_api" {
+       api_key_source               = (known after apply)
+       arn                          = (known after apply)
+       binary_media_types           = (known after apply)
+       body                         = <<~EOT
            array:
-               value1
-               value2
        EOT
+       created_date                 = (known after apply)
+       description                  = (known after apply)
+       disable_execute_api_endpoint = (known after apply)
+       execution_arn                = (known after apply)
+       id                           = (known after apply)
+       minimum_compression_size     = -1
+       name                         = "test"
+       policy                       = (known after apply)
+       root_resource_id             = (known after apply)
+       tags_all                     = (known after apply)

+       endpoint_configuration {
+           types            = [
+               "REGIONAL",
            ]
+           vpc_endpoint_ids = (known after apply)
        }
    }

Logs

Not available.

Environment details

• Atlantis version: 0.17.4
• Atlantis flags:

ATLANTIS_ENABLE_DIFF_MARKDOWN_FORMAT=true

[jamengual]

is this still happening with v0.19.8?

[dyurovskykh-tivo]

Hello, @jamengual,

Diff looks much better with v0.19.8 but there is still unexpected diff:

Thanks!

[jamengual]

are you using ? enable-diff-markdown-format: true

[dyurovskykh-tivo]

Yes, we have env var ATLANTIS_ENABLE_DIFF_MARKDOWN_FORMAT set to true

[jamengual]

this one is hard to fix since you could have anything on that yaml.

I think there is always going to be some discrepancies

[dyurovskykh-tivo]

Any idea why [ was added to the regex - https://github.com/runatlantis/atlantis/blob/master/server/events/models/models.go#L387?

If you drop it like:

- diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\)|\[)(.*)`)
+ diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\))(.*)`)

The diff becomes correct (https://go.dev/play/p/uTH9ogZJcCe). Note: test TestRenderProjectResultsWithEnableDiffMarkdownFormat passes with ^ change too.

[jamengual]

no idea, that was added a while ago

…

On Tue, Sep 27, 2022 at 12:12 AM Dmytro Yurovskykh ***@***.***> wrote: Any idea why [ was added to the regex - https://github.com/runatlantis/atlantis/blob/master/server/events/models/models.go#L387 ? If you drop it like: - diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\)|\[)(.*)`)+ diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\))(.*)`) The diff becomes correct. Note: test TestRenderProjectResultsWithEnableDiffMarkdownFormat passes with ^ change too. — Reply to this email directly, view it on GitHub <#1899 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERF6BGLOOW2OLCSYSKDWAKM7FANCNFSM5IB62SRA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[jamengual]

you can send a PR if you think that is a better regex. On Tue, Sep 27, 2022 at 12:25 AM Pepe (Jose) Amengual < ***@***.***> wrote:

…

no idea, that was added a while ago On Tue, Sep 27, 2022 at 12:12 AM Dmytro Yurovskykh < ***@***.***> wrote: > Any idea why [ was added to the regex - > https://github.com/runatlantis/atlantis/blob/master/server/events/models/models.go#L387 > ? > > If you drop it like: > > - diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\)|\[)(.*)`)+ diffKeywordRegex := regexp.MustCompile(`(?m)^( +)([-+~]\s)(.*)(\s=\s|\s->\s|<<|\{|\(known after apply\))(.*)`) > > The diff becomes correct. Note: test > TestRenderProjectResultsWithEnableDiffMarkdownFormat passes with ^ > change too. > > — > Reply to this email directly, view it on GitHub > <#1899 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAQ3ERF6BGLOOW2OLCSYSKDWAKM7FANCNFSM5IB62SRA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

[dyurovskykh-tivo]

Sure, I've asked an engineer from my team to work on the PR.

[jamengual]

no, I tried that before and I can only assign to people on the maintainer group, sorry

…

On Tue, Sept 27, 2022, 5:33 a.m. VladimirSt-TPE ***@***.***> wrote: Could you assign the issue to me? — Reply to this email directly, view it on GitHub <#1899 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQ3ERANSB4MZ5UTHTP7MITWALSQLANCNFSM5IB62SRA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[pauloconnor]

Any idea why [ was added to the regex - https://github.com/runatlantis/atlantis/blob/master/server/events/models/models.go#L387?

I think I added it to catch situations where the change is like

+           types            = [
+               "REGIONAL",
            ]

so it'd catch the types = [ line. This whole regex is annoying. Wish Golang let us use lookaheads, though I understand why it doesn't

[dyurovskykh-tivo]

@pauloconnor
Looks like this type of pattern works fine even without [ in the regex:

Result:

If you are aware of other patterns to be covered/tested, please, let @VladimirSt-XP know and he will try adding it to his #2551

[nitrocode]

We probably need a test for the types = [] to ensure the regex handles it correctly.

It would help if we commented the regex too so each significant character has a reason for being there.

[nitrocode]

Does the github diff markdown work now for embedded YAML content ? @dyurovskykh-tivo @pauloconnor

[dyurovskykh-tivo]

@nitrocode It should work for anything covered in tests but I am not sure it covers everything. Consider adding more tests to cover more use cases.

[nitrocode]

Sorry, my question was more for the lack of functionality described in the original post above and not the tests themselves.

Is the markdown formatting working properly now in the latest atlantis for embedded YAML? or should we keep this open? What are the other edge cases?

[dyurovskykh-tivo]

ah, I think this should be fixed by #2551