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 development scripts #2928

Merged
merged 23 commits into from Sep 21, 2023
Merged

Conversation

WillAbides
Copy link
Contributor

@WillAbides WillAbides commented Sep 12, 2023

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
Copy link

codecov bot commented Sep 12, 2023

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
Collaborator

gmlewis commented Sep 18, 2023

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
Contributor Author

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
Collaborator

gmlewis commented Sep 18, 2023

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! 😄

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Sep 18, 2023
Copy link
Collaborator

@gmlewis gmlewis left a comment

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.

CONTRIBUTING.md Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
WillAbides and others added 2 commits September 18, 2023 11:04
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Copy link
Collaborator

@gmlewis gmlewis left a comment

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.

@gmlewis gmlewis changed the title Development scripts Add development scripts Sep 18, 2023
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 21, 2023

Thank you, @valbeat !

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

@WillAbides
Copy link
Contributor Author

Thanks. I'll update it in the morning.

@WillAbides
Copy link
Contributor Author

@gmlewis It's ready to go now

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 21, 2023
Copy link
Collaborator

@gmlewis gmlewis left a comment

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:master Sep 21, 2023
7 checks passed
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.

Proposal: development scripts
3 participants