[HPR-1190] Add `packer-sdc fix` command #190

nywilken · 2023-06-22T23:33:25Z

In an effort to automate the temporary fix, and its removal in the future, a fix sub-command has been added to the packer-sdc command; not to be confused with packer fix which fixes legacy JSON template files.

By running the packer-sdc fix command within the plugin's root project directory the command will check the plugin's go.mod file has a replace directive to the go-cty fork. If no replace directive is found it will be automatically added to the go.mod file.

Depends on #189
Relates to: #187
Closes #135

Example Outputs

Help Output

~>  ./packer-sdc fix -h

Usage: packer-sdc fix [options] directory

  Fix rewrites parts of the plugin codebase to address known issues or
  common workarounds used within plugins consuming the Packer plugin SDK.

Options:
  -diff         If the -diff flag is set fix prints the differences an applied fix would introduce.

Available fixes:
  gocty         Adds a replace directive for github.com/zclconf/go-cty to github.com/nywilken/go-cty

Basic Diff

~>  ./packer-sdc fix -diff ../../../packer/
--- /home/vagrant/Development/packer/go.mod
+++ /home/vagrant/Development/packer/go.modfixed
@@ -213,3 +213,5 @@
 )

 go 1.20
+
+replace github.com/zclconf/go-cty => github.com/nywilken/go-cty v1.12.1 // added by packer-sdc fix as noted in github.com/hashicorp/packer-plugin-sdk/issues/187

Applied Fix

~>  ./packer-sdc fix ../../../packer/
/home/vagrant/Development/packer/go.mod
~>  git diff
diff --git a/go.mod b/go.mod
index fa48d7d58..02b455992 100644
--- a/go.mod
+++ b/go.mod
@@ -213,3 +213,5 @@ require (
 )

 go 1.20
+
+replace github.com/zclconf/go-cty => github.com/nywilken/go-cty v1.12.1 // added by packer-sdc fix as noted in github.com/hashicorp/packer-plugin-sdk/issues/187

lbajolet-hashicorp

Couple nits, but the code looks good to me overall!

cmd/packer-sdc/internal/fix/fix.go

cmd/packer-sdc/internal/fix/gocty.go

cmd/packer-sdc/internal/fix/cmd.go

lbajolet-hashicorp

Just left a couple of nits/questions, but the current implementation looks good to me.
I see this is still a draft so I'll wait until this is final to approve, but I'd think this is almost ready to merge.

cmd/packer-sdc/internal/fix/cmd.go

cmd/packer-sdc/internal/fix/gocty.go

cmd/packer-sdc/internal/fix/fix.go

Fix rewrites parts of the plugin codebase to address known issues or common workarounds used within plugins consuming the Packer plugin SDK. ``` ~> ./packer-sdc fix -check ../../../packer-plugin-tencentcloud Found the working directory /Users/wilken/Development/linux-dev/packer-plugin-tencentcloud gocty Unfixed! ```

sylviamoss · 2023-07-14T11:44:41Z

cmd/packer-sdc/internal/fix/README.md

+Available Fixes:
+
+ gocty
+           Adds a replace directive for github.com/zclconf/go-cty to github.com/nywilken/go-cty


Nice readme! Should the issue be linked here as well?

Good call out. I will add that to the README.

Added let me know what you think.

sylviamoss · 2023-07-14T11:48:59Z

cmd/packer-sdc/internal/fix/cmd.go

+		//matches contains all files to apply the said fix on
+		for _, filename := range matches {
+			if _, ok := srcFiles[filename]; !ok {
+				// TODO handle error


There's a TODO here

TODO handled. Thanks for catching that.

sylviamoss · 2023-07-14T11:50:56Z

cmd/packer-sdc/internal/fix/cmd.go

+				srcFiles[filename] = bytes.Clone(bs)
+			}
+
+			fixedData, ok := fixedFiles[filename]


Why you fix the data again?

So it is not really an issue since we have one fix but if there are multiple fixes that would get applied to the same file we need to make sure that the fixes are accumulative and not modifying the same unfixed src file.

To do ☝️ I store the already fixed data into the fixedFiles map and before applying another fix I check if the contents of the file have changed if so I grab the fixed file before applying the next fix. If we've never seen the file before or it has not been fixed previously we copy the original source into fixedData and only store within the fixedFiles map if a fix has been applied.

Please let me know if I explained that well or if you're still unclear on why

oh yeah, I see now! First time I looked I didn't realized that fixedFiles is used for all fixes. Makes sense to me, thanks for explaining!

sylviamoss · 2023-07-14T11:59:25Z

cmd/packer-sdc/internal/fix/gocty.go

+		return nil, fmt.Errorf("%s: failed to drop previously added replacement fix %v", modFilePath, err)
+	}
+
+	commentSuffix := " // added by packer-sdc fix as noted in github.com/hashicorp/packer-plugin-sdk/issues/187"


lbajolet-hashicorp

@sylviamoss found some good things to point out, and there's still the outstanding error comment that might need addressing, but overall this looks good to me!

I'm approving this right now, when you addressed the remaining comments this is good to go imho.

cmd/packer-sdc/internal/fix/cmd.go

sylviamoss

Very good command, ready to implement more fixes. 👍🏼 LGTM!

This change is major refactor of the fix command, and it's underlying fixer interface. This change adds support for the `-diff` flag which displays the diff between the unfixed and fixed files, if available. Along with the refactor the ability to apply multiple fixes to the same file without potential write conflicts where previous changes were removed after reprocessing. * Add a scan function that returns a list of files to apply a fix on to provide flexibility in the future for collecting a list of files. * Replace cmp.Diff with github.com/pkg/diff for better file diffs * Return error when missing directory argument * Add error handling when trying to read any scanned files

nywilken changed the base branch from main to nywilken/go-cty-fork June 22, 2023 23:33

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from 7feee45 to c119a3a Compare June 22, 2023 23:34

nywilken added this to the 0.5.0 milestone Jul 3, 2023

nywilken added the enhancement New feature or request label Jul 3, 2023

nywilken marked this pull request as ready for review July 3, 2023 18:40

nywilken requested a review from a team as a code owner July 3, 2023 18:40

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from 103ddd7 to cf5b6a2 Compare July 3, 2023 19:43

nywilken marked this pull request as draft July 4, 2023 01:24

lbajolet-hashicorp reviewed Jul 4, 2023

View reviewed changes

cmd/packer-sdc/internal/fix/fix.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/gocty.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/gocty.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/gocty.go Outdated Show resolved Hide resolved

Base automatically changed from nywilken/go-cty-fork to main July 5, 2023 14:47

nywilken force-pushed the nywilken/packer-sdc_fix_command branch 2 times, most recently from 2989a74 to 20978e7 Compare July 7, 2023 19:35

nywilken requested a review from lbajolet-hashicorp July 7, 2023 19:41

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from 20978e7 to 960f4cb Compare July 7, 2023 20:07

nywilken commented Jul 7, 2023

View reviewed changes

cmd/packer-sdc/internal/fix/cmd.go Outdated Show resolved Hide resolved

lbajolet-hashicorp reviewed Jul 7, 2023

View reviewed changes

cmd/packer-sdc/internal/fix/cmd.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/gocty.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/gocty.go Outdated Show resolved Hide resolved

cmd/packer-sdc/internal/fix/fix.go Outdated Show resolved Hide resolved

nywilken marked this pull request as ready for review July 13, 2023 19:38

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from cacfbef to b72c7a6 Compare July 13, 2023 21:10

sylviamoss reviewed Jul 14, 2023

View reviewed changes

lbajolet-hashicorp approved these changes Jul 14, 2023

View reviewed changes

cmd/packer-sdc/internal/fix/cmd.go Outdated Show resolved Hide resolved

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from 4de2695 to 62235b2 Compare July 14, 2023 14:29

sylviamoss approved these changes Jul 14, 2023

View reviewed changes

nywilken force-pushed the nywilken/packer-sdc_fix_command branch from 1c98d1d to 5e36021 Compare July 14, 2023 16:58

nywilken merged commit 7d3a4b2 into main Jul 14, 2023
5 checks passed

nywilken deleted the nywilken/packer-sdc_fix_command branch July 14, 2023 17:14

RodrigoCMoraes mentioned this pull request May 29, 2024

sync/upstream + upgrade/security inloco/packer-plugin-sdk#34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HPR-1190] Add `packer-sdc fix` command #190

[HPR-1190] Add `packer-sdc fix` command #190

nywilken commented Jun 22, 2023 •

edited

Loading

lbajolet-hashicorp left a comment

lbajolet-hashicorp left a comment

sylviamoss Jul 14, 2023

nywilken Jul 14, 2023

nywilken Jul 14, 2023

sylviamoss Jul 14, 2023

nywilken Jul 14, 2023

sylviamoss Jul 14, 2023

nywilken Jul 14, 2023

sylviamoss Jul 14, 2023

sylviamoss Jul 14, 2023

lbajolet-hashicorp left a comment

sylviamoss left a comment

[HPR-1190] Add packer-sdc fix command #190

[HPR-1190] Add packer-sdc fix command #190

Conversation

nywilken commented Jun 22, 2023 • edited Loading

Example Outputs

Help Output

Basic Diff

Applied Fix

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

sylviamoss left a comment

Choose a reason for hiding this comment

[HPR-1190] Add `packer-sdc fix` command #190

[HPR-1190] Add `packer-sdc fix` command #190

nywilken commented Jun 22, 2023 •

edited

Loading