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

add 012 upgrade option #6

Merged
merged 7 commits into from
Jan 7, 2020
Merged

Conversation

megan07
Copy link
Contributor

@megan07 megan07 commented Oct 31, 2019

./terrafmt upgrade012 path/to/test_files.go

Because terraform 0.12upgrade needs to be run on a directory and doesn't have a stdin option, it will write the blocks to a buffer, create a temp directory, write the buffer to a temp file there, run terraform 0.12upgrade on the temp directory, read the temp file back to the format block before it writes it to the real file.

I didn't implement the fmtcompat option into this. I didn't entirely understand the use case, but I can add it in if you need me to.

lib/upgrade012/upgrade.go Show resolved Hide resolved
)

//todo list of them?
const fmtVerbCompatibilityDelimiter = "@@_@@ TFMT"
Copy link
Contributor

Choose a reason for hiding this comment

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

fmtVerbCompatibilityDelimiter is unused (from deadcode)


if err != nil {
if stdout != nil {
fmt.Println(stdout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error return value of fmt.Println is not checked (from errcheck)

cli/cmds.go Outdated
return err
}

br.Writer.Write([]byte(fb))
Copy link
Contributor

Choose a reason for hiding this comment

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

Error return value of br.Writer.Write is not checked (from errcheck)


// handle bare %s
// figure out why the * doesn't match both later
b = string(regexp.MustCompile(`(?m:^%[sdfgtqv]$)`).ReplaceAllString(b, `#@@_@@ TFMT:$0`))
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary conversion (from unconvert)

// strip it here
fb := strings.TrimSuffix(string(raw), "\n")

return string(fb), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

unnecessary conversion (from unconvert)

lib/upgrade012/fmtverbs.go Show resolved Hide resolved
lib/upgrade012/upgrade.go Show resolved Hide resolved
cli/cmds.go Show resolved Hide resolved
cli/cmds.go Show resolved Hide resolved
Copy link
Owner

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @megan07, just left a couple minor comments inline

cli/cmds.go Outdated
Comment on lines 119 to 121
_, err = br.Writer.Write([]byte(fb))

if err == nil && fb != b {
Copy link
Owner

Choose a reason for hiding this comment

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

These two lines could be merged:

					if _, err = br.Writer.Write([]byte(fb)); err == nil && fb != b {

cli/cmds.go Outdated
// nolint staticcheck
fmt.Fprintf(os.Stderr, c.Sprintf("<%s>%s</>: <cyan>%d</> lines & formatted <yellow>%d</>/<yellow>%d</> blocks!\n", fc, br.FileName, br.LineCount, blocksFormatted, br.BlockCount))
}
if err != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be after br.DoTheThing(filename)?

if err != nil {
return err
}
return nil
Copy link
Owner

Choose a reason for hiding this comment

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

could we add the new check for the diff function here were it does an os.exit(-1) if there are any blocks with error?

lib/blocks/blockreader.go Show resolved Hide resolved
lib/upgrade012/upgrade.go Show resolved Hide resolved
@megan07 megan07 requested a review from katbyte January 7, 2020 22:30
Copy link
Owner

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @megan07! LGTM 🚀

@katbyte katbyte merged commit 72d6720 into katbyte:master Jan 7, 2020
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.

3 participants