Skip to content

migrations: SQL formatting#1782

Merged
github-actions[bot] merged 1 commit intoquay:mainfrom
hdonnay:feature/not-affected/sql-formatting
Mar 25, 2026
Merged

migrations: SQL formatting#1782
github-actions[bot] merged 1 commit intoquay:mainfrom
hdonnay:feature/not-affected/sql-formatting

Conversation

@hdonnay
Copy link
Copy Markdown
Member

@hdonnay hdonnay commented Mar 19, 2026

This change adds a generate directive that runs an SQL formatter via go run. It also runs the formatter over the existing SQL.

@hdonnay hdonnay requested a review from a team as a code owner March 19, 2026 21:40
return nil
}

//go:generate find . -name *.sql -exec go run github.com/wasilibs/go-sql-formatter/v15/cmd/sql-formatter@latest --language postgresql --fix {} ;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the only "real" change, everything else is whitespace changes as a result of running go generate.

@hdonnay hdonnay added this to the Not Affected milestone Mar 19, 2026
@hdonnay hdonnay force-pushed the feature/not-affected/sql-formatting branch from 7172a7e to a3c8df5 Compare March 24, 2026 19:05
Copy link
Copy Markdown

@dcaravel dcaravel left a comment

Choose a reason for hiding this comment

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

LGTM - can't say I agree with all the formatting decisions made by the lib, but that's a me problem ;)

return nil
}

//go:generate find . -name *.sql -exec go run github.com/wasilibs/go-sql-formatter/v15/cmd/sql-formatter@latest --language postgresql --fix {} ;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For consideration - is it worth referencing a specific tag vs. latest here? Thinking of risk if repo is compromised, a tag + sumdb may help detect.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So, this tool recommends against using go tool for dependency tree reasons.
I'd be wary of pinning a tag because then there's no good way to keep it updated.
Considering that its use is completely offline (we're checking in and reviewing the output) and any exploit would need to be in the wasm runtime or defeat the wasm sandbox, I think it's very low risk to just use latest.

Copy link
Copy Markdown

@dcaravel dcaravel Mar 25, 2026

Choose a reason for hiding this comment

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

and any exploit would need to be in the wasm runtime or defeat the wasm sandbox, I think it's very low risk to just use latest.

Fair enough - counter-point: If that repo were compromised the attacker would likely remove the Wasm sandbox entirely and add malicious code, which would run directly on our machines during go generate.

This change adds a `generate` directive that runs an SQL formatter via
`go run`. It also runs the formatter over the existing SQL.

See-also: https://issues.redhat.com/browse/CLAIRDEV-85
Change-Id: I7ae14f596154b6f9bec66800c388bfe5ce732ca4
Signed-off-by: Hank Donnay <hdonnay@redhat.com>
@hdonnay hdonnay force-pushed the feature/not-affected/sql-formatting branch from a3c8df5 to f00684c Compare March 25, 2026 14:05
@hdonnay
Copy link
Copy Markdown
Member Author

hdonnay commented Mar 25, 2026

LGTM - can't say I agree with all the formatting decisions made by the lib[.]

Honestly me neither, but I think consistency wins over my formatting peccadilloes. It's like the go proverb:

Gofmt's style is no one's favorite, yet gofmt is everyone's favorite.

@hdonnay
Copy link
Copy Markdown
Member Author

hdonnay commented Mar 25, 2026

/fast-forward

@github-actions github-actions bot merged commit f00684c into quay:main Mar 25, 2026
6 checks passed
@hdonnay hdonnay deleted the feature/not-affected/sql-formatting branch March 25, 2026 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants