-
Notifications
You must be signed in to change notification settings - Fork 46
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
[HPR-1190] Add packer-sdc fix
command
#190
Conversation
7feee45
to
c119a3a
Compare
103ddd7
to
cf5b6a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couple nits, but the code looks good to me overall!
2989a74
to
20978e7
Compare
20978e7
to
960f4cb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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! ```
cacfbef
to
b72c7a6
Compare
Available Fixes: | ||
|
||
gocty | ||
Adds a replace directive for github.com/zclconf/go-cty to github.com/nywilken/go-cty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice readme! Should the issue be linked here as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call out. I will add that to the README.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added let me know what you think.
cmd/packer-sdc/internal/fix/cmd.go
Outdated
//matches contains all files to apply the said fix on | ||
for _, filename := range matches { | ||
if _, ok := srcFiles[filename]; !ok { | ||
// TODO handle error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a TODO here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO handled. Thanks for catching that.
srcFiles[filename] = bytes.Clone(bs) | ||
} | ||
|
||
fixedData, ok := fixedFiles[filename] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why you fix the data again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
4de2695
to
62235b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
1c98d1d
to
5e36021
Compare
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 withpacker 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
Basic Diff
Applied Fix