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

CI Enhancements #38

Merged
merged 9 commits into from
Oct 11, 2024
Merged

CI Enhancements #38

merged 9 commits into from
Oct 11, 2024

Conversation

systay
Copy link
Contributor

@systay systay commented Oct 10, 2024

Description

This PR introduces several key improvements to the CI workflow, focusing on better formatting checks, dependency management, and enhanced test result visibility.

Changes:

  1. Code Formatting Enforcement (make pretty):

    • Added a make pretty step to the CI workflow that runs gofumpt and goimports-reviser to ensure Go code is consistently formatted.
    • The CI now fails if code is not properly formatted, ensuring all commits maintain a consistent style.
  2. Go Module Tidy Check:

    • Integrated a go mod tidy check to ensure go.mod and go.sum files are up to date and clean.
    • CI will fail if go mod tidy results in any changes, prompting developers to keep dependencies tidy before merging.
  3. Improved Test Reporting:

    • Replaced the basic make test step with a detailed test run using go test -v, which outputs detailed logs for debugging.
    • Added the dorny/test-reporter GitHub Action to annotate test failures directly in the GitHub UI using the JUnit format, making it easier to view and track failed tests without manually parsing logs.
  4. Linting:

    • Added make lint and a linter configuration.
  5. Pre-commit Git Hook:

    • Adds a git pre-commit hook when you run make install-hooks.

Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
@systay systay changed the title more data and queries Add CI Enhancements: Code Formatting, Module Tidy Check, and Improved Test Reporting Oct 10, 2024
@systay systay changed the title Add CI Enhancements: Code Formatting, Module Tidy Check, and Improved Test Reporting CI Enhancements Oct 10, 2024
Signed-off-by: Andres Taylor <[email protected]>
@systay systay marked this pull request as ready for review October 10, 2024 09:26
Copy link

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

Nice! Always love more testing and more formalization.

pretty: install-tools
@echo "Running formatting tools..."
@gofumpt -l -w . >/dev/null 2>&1 || true

Choose a reason for hiding this comment

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

Is there a reason why not to use the same gofmt (or rather goimports) that vitessio/vitess uses?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... I think gofumpt is superior to gofmt, and goimports-reviser is better than goimports. I wish Vitess would change, and not the other way around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

also @shlomi-noach, I think both of these are also supersets of what vitessio/vitess uses, i.e. the code will pass the scrutiny of gofmt and goimports with no change

Copy link
Member

Choose a reason for hiding this comment

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

I think it's reasonable to use both new tools

Copy link
Member

Choose a reason for hiding this comment

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

we can always try to use these two new tools on vt and get a better opinion after testing it for a while, helping decide if we want to use the same on vitess

Copy link

@mattlord mattlord left a comment

Choose a reason for hiding this comment

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

❤️

Copy link
Member

@GuptaManan100 GuptaManan100 left a comment

Choose a reason for hiding this comment

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

LGTM!

@systay systay merged commit e430093 into main Oct 11, 2024
3 checks passed
@systay systay deleted the ci branch October 11, 2024 06:50
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.

6 participants