Skip to content

Add development scripts - #2928

Merged
gmlewis merged 23 commits into
google:masterfrom
WillAbides:scripts-to-rule-them-all
Sep 21, 2023
Merged

Add development scripts#2928
gmlewis merged 23 commits into
google:masterfrom
WillAbides:scripts-to-rule-them-all

Conversation

@WillAbides

@WillAbidesWillAbides commented Sep 12, 2023

Copy link
Copy Markdown
Contributor

closes#2927

Scripts

This adds the four scripts proposed in #2927.

The scripts all follow the basic pattern of iterating over the go modules in the repo and running the appropriate commands in each directory. Because of this, there is some duplicate code. I chose to keep it this way because deduping it added complexity, and I wanted to keep these scripts as simple as possible to understand.

The only dependencies for these scripts are git and a posix shell.

  • script/fmt.sh is the simplest one. It runs go fmt ./... in each module directory.

  • script/generate.sh runs go generate ./... and go mod tidy -compat '1.17' in each module directory. It also has a --check flag which causes it to run on a temporary git worktree and fail if any new diff is created.

  • script/lint.sh runs golangci-lint run on each module then runs script/generate.sh --check. It installs golangci-lint in bin/ if needed.

  • script/test.sh runs go test in each module directory. If arguments are provided, those are used as arguments for go test, otherwise it uses the default arguments -race -covermode atomic ./...

script/lint.sh and script/test.sh skip the module in example/newreposecretwithlibsodium because it cannot compile unless libsodium is available.

CONTRIBUTING.md

  • Updates CONTRIBUTING.md to include the new scripts and generally updates the "Submitting a patch" section.

Workflow changes

linter.yml

  • Runs script/lint.sh instead of using the golangci-lint action.
  • No longer uses a run matrix. Because script/lint.sh iterates over modules, I was able to remove the run matrix. We now only run one lint job instead of four.
  • Stale generated files will now be reported on lint runs because script/lint.sh validates go generate.

tests.yml

  • Runs script/test.sh instead of running go test multiple times in separate steps.
  • No longer checks go generate now that it is checked in lint.
  • Only sets -coverprofile when codecov will be updated because -coverprofile prevents cached test results from being used.
  • Sets default shell to bash so that it doesn't need to be specified on each step.

Misc changes

Running script/test.sh exposed an issue where the examples module won't compile on Windows because syscall.Stdin is typed differently on Windows. I resolved this by replacing syscall.Stdin with int(os.Stdin.Fd())

@codecov

codecovBot commented Sep 12, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2928 (13c9ed3) into master (b45ef89) will not change coverage.
The diff coverage is n/a.

@@ Coverage Diff @@## master #2928 +/- ##
=======================================
Coverage 98.17% 98.17% =======================================
Files 143 143 Lines 12597 12597 =======================================
Hits 12367 12367 Misses 156 156 Partials 74 74 

@WillAbides
WillAbides marked this pull request as ready for review September 15, 2023 17:56
@gmlewis

Copy link
Copy Markdown
Collaborator

Would it bother/upset you if I asked you to rename all the bash scripts in the "scripts" directory to have a ".sh" suffix?
This is a convention I've been using for decades, and I'd love to continue that, but won't be heart-broken if you object.

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

I can go either way on script names. I noticed .gitignore has a *.sh line. I'll remove that altogether because I suspect it isn't necessary anymore. If that causes a problem I can restore it and add !/scripts/*.sh

@gmlewis

Copy link
Copy Markdown
Collaborator

I can go either way on script names. I noticed .gitignore has a *.sh line. I'll remove that altogether because I suspect it isn't necessary anymore. If that causes a problem I can restore it and add !/scripts/*.sh

Ah, yes, please leave the *.sh and add your workaround, as I have a slew of hacky bash scripts in the top-level that help me maintain this repo. Thank you! 😄

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 18, 2023

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great, @WillAbides !
Thank you so much for doing this! I think it will help users and maintainers alike.

While you are touching parts of CONTRIBUTING.md, I hope you don't mind if I ask you to tweak a couple things that aren't actually related to your PR? That would be awesome.

Comment threadCONTRIBUTING.md Outdated
Comment threadCONTRIBUTING.md Outdated
WillAbidesand others added 2 commits September 18, 2023 11:04
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Comment threadCONTRIBUTING.md Outdated
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Excellent, @WillAbides - thank you!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewisgmlewis changed the title Development scriptsAdd development scriptsSep 18, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @valbeat !

@WillAbides - do you want to change "scripts" back to your original "script" before I merge?

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

Thanks. I'll update it in the morning.

@WillAbides

Copy link
Copy Markdown
ContributorAuthor

@gmlewis It's ready to go now

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 21, 2023

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @WillAbides !
LGTM.
Merging.

@gmlewis
gmlewis merged commit 9f1382e into google:masterSep 21, 2023
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto 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.

Proposal: development scripts

3 participants

@WillAbides@gmlewis@valbeat