dependabot-core icon indicating copy to clipboard operation
dependabot-core copied to clipboard

Add Terraform JSON support

Open melendezd opened this issue 3 years ago • 3 comments

This PR addresses #4080

Adding support for updating Terraform JSON files with .tf.json file extension. Most of the tests present for .tf files are duplicated for .tf.json files (with some restructuring of file_updater_spec.rb to make this easier and less repetitious) since the functionality should be essentially the same for the two formats.

Currently, the modified file updater pulls the JSON into an object, modifies the object, and JSON.pretty_generate's it out to the file. This may change the formatting of the original JSON file (tab width, for example). I'm not sure if this is okay or if there is a better approach to avoid this issue.

Additionally, I noticed that a couple tests file_updater_spec.rb check the content of the original file rather than the output of the file updater, creating a test that will pass no matter what the updater is doing. I'm not sure if I'm misunderstanding but I feel it's worth mentioning. Here's a snippet from main: https://github.com/dependabot/dependabot-core/blob/ca9f236591ba49fa6e2a8d5f06e538614033a628/terraform/spec/dependabot/terraform/file_updater_spec.rb#L810-L830

and the corresponding snippet from my PR: https://github.com/yoyomeng2/dependabot-core/blob/f6258ad94f53360c2b39920ff6303bcfcfb03094/terraform/spec/dependabot/terraform/file_updater_spec.rb#L887-L894

Finally, I'm unsure if there's more verification that needs to be done other than the unit tests before being able to merge.

Any help or feedback is greatly appreciated. Thanks!

melendezd avatar Jun 20 '22 21:06 melendezd

I've updated the file updater to use regex substitutions in the JSON files. The provider declaration regex was quite difficult because the source and version can be declared in either order, but I believe my approach works as long as there aren't any references to named values (like ${var.name}) after the version and before the source. It seems like the case of the version coming before the source isn't handled in the hcl format updater, so I'm wondering how important this is.

melendezd avatar Jun 28 '22 13:06 melendezd

@melendezd do you mind rebasing and also squashing down some of the smaller commits? We merge PR's with a merge strategy currently, not a squash strategy, so what's in the PR history is what eventually gets committed.

To be clear: No need to squash to only a single commit--if there are multiple logical changes, feel free to keep them separate for easier review, but I suspect there's not 32 separate logical change sets here... 😀

Tidying this up will also make it easier to review the PR.

jeffwidman avatar Sep 12 '22 17:09 jeffwidman

gentle nudge @melendezd

jeffwidman avatar Sep 22 '22 09:09 jeffwidman

gentle nudge @melendezd

My bad, this got away from me. I'm a bit busy at the moment but will let you know when I get back on this.

melendezd avatar Sep 26 '22 14:09 melendezd

@melendezd any chance things have slowed down enough to get this across the finish line?

jeffwidman avatar Nov 23 '22 19:11 jeffwidman

How's this look? :)

melendezd avatar Nov 28 '22 18:11 melendezd

@jeffwidman - Are there any further updates necessary for this to be put on the schedule for being reviewed?

njculver avatar Feb 13 '23 19:02 njculver

I started to have a look:

  • Some fixtures seem unused: unparseable_json, provider_implicit_source_json, git_tags_013_json.
  • Needs a rebase since this conflicts with #6537.

deivid-rodriguez avatar Feb 14 '23 19:02 deivid-rodriguez