Fix up the setup #179
Fix up the setup #179
Conversation
- Bump the strmangle dependency from `v0.0.4` to `v0.0.6` - Remove the `sqlboiler` v3, only v4 should be used in the project - Update README for clean instructions on the `seeding` the entities - Update Dockerfile - The docker build will now be quicker due to the caching of dependency download
- Reformat the `README.md`
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the Dockerfile, Go module management, error handling, and test cases. Key modifications include optimizing the Docker build process for a Go application, updating dependency versions in Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (7)
scripts/setup-pre-commit.sh (1)
Line range hint
1-24: Consider improving overall script robustnessWhile not directly related to the recent changes, there are a few areas where the script could be improved:
- The script assumes
brewandgoare installed without checking.- It doesn't handle potential errors from individual tool installations.
- There's a mix of package managers (brew and go install) which might not be ideal for all environments.
Consider the following improvements:
Add checks for required tools:
command -v brew >/dev/null 2>&1 || { echo "brew is required but not installed. Aborting." >&2; exit 1; } command -v go >/dev/null 2>&1 || { echo "go is required but not installed. Aborting." >&2; exit 1; }Handle potential errors for each installation:
install_tool() { if ! $@; then echo "Failed to install $1. Aborting." >&2 exit 1 fi } install_tool brew install pre-commit install_tool brew install golangci-lint # ... repeat for other installationsConsider using a single package manager or providing alternative installation methods for non-macOS systems.
These changes would make the script more robust and portable across different environments.
Dockerfile (1)
22-25: Good consolidation of build commands, but consider further optimizations.Consolidating the build commands into a single RUN instruction is a good practice as it reduces the number of layers in the final image. However, there are a couple of points to consider:
Running the seeder (
go run ./cmd/seeder/main.go) before building might not be necessary in the Dockerfile. Consider if this step is required during the build process or if it should be part of the application startup.To improve build caching and make debugging easier, you could split the build commands into separate RUN instructions, each building one executable. This approach allows Docker to cache each step separately, potentially speeding up rebuilds when only one part of the application changes.
Here's a suggested refactor:
RUN go build -o ./output/server ./cmd/server/main.go RUN go build -o ./output/migrations ./cmd/migrations/main.go RUN go build -o ./output/seeder ./cmd/seeder/exec/seed.goThis way, if you change only the server code, Docker can use the cached layers for migrations and seeder, rebuilding only the server.
pkg/utl/rediscache/service_test.go (5)
110-112: Consider simplifying error formattingWhile the change to
fmt.Errorf("%s", ...)improves consistency, it's unnecessary when the argument is already a string. You can simplify this to:-errMsg: fmt.Errorf("%s", ErrMsgGetKeyValue), +errMsg: errors.New(ErrMsgGetKeyValue), -conn.Command("GET", fmt.Sprintf("user%d", args.userID)).ExpectError(fmt.Errorf("%s", ErrMsgGetKeyValue)) +conn.Command("GET", fmt.Sprintf("user%d", args.userID)).ExpectError(errors.New(ErrMsgGetKeyValue))This maintains the same functionality while being more concise.
138-138: LGTM with a minor suggestionThe change to
*gomonkey.Patcheson line 138 is consistent with earlier changes and improves type safety. However, the error formatting on line 158 can be simplified:-errMsg: fmt.Errorf("%s", ErrMsgSetKeyValue), +errMsg: errors.New(ErrMsgSetKeyValue),This maintains functionality while being more concise.
Also applies to: 158-158
167-167: LGTM with a minor suggestionThe change to
*gomonkey.Patcheson line 167 is consistent with earlier changes and improves type safety. However, the error formatting on line 184 can be simplified:-errMsg: fmt.Errorf("%s", ErrMsgSetKeyValue), +errMsg: errors.New(ErrMsgSetKeyValue),This maintains functionality while being more concise.
Also applies to: 184-184
378-379: Consistent changes with room for simplificationThe changes to
*gomonkey.Patchesand error handling are consistent with earlier modifications. However, all instances offmt.Errorf("%s", ...)can be simplified:-conn.Command("GET", fmt.Sprintf("role%d", args.roleID)).ExpectError(fmt.Errorf("%s", ErrMsgGetKeyValue)) +conn.Command("GET", fmt.Sprintf("role%d", args.roleID)).ExpectError(errors.New(ErrMsgGetKeyValue)) -return fmt.Errorf("%s", ErrMsgUnmarshal) +return errors.New(ErrMsgUnmarshal) -conn.Command("GET", fmt.Sprintf("role%d", args.roleID)).ExpectError(fmt.Errorf("there was an error")) +conn.Command("GET", fmt.Sprintf("role%d", args.roleID)).ExpectError(errors.New("there was an error"))These changes maintain functionality while being more concise.
Also applies to: 383-385, 389-389, 394-394
454-454: Consistent changes with room for simplificationThe changes to error handling are consistent with earlier modifications. However, all instances of
fmt.Errorf("%s", ...)can be simplified:-errMsg: fmt.Errorf("%s", ErrMsgFromRedisDial), +errMsg: errors.New(ErrMsgFromRedisDial), -return nil, fmt.Errorf("%s", ErrMsgFromRedisDial) +return nil, errors.New(ErrMsgFromRedisDial) -errMsg: fmt.Errorf("%s", ErrMsgFromConnDo), +errMsg: errors.New(ErrMsgFromConnDo), -return nil, fmt.Errorf("%s", ErrMsgFromConnDo) +return nil, errors.New(ErrMsgFromConnDo) -errMsg: fmt.Errorf("%s", ErrMsgFromRedisDial), +errMsg: errors.New(ErrMsgFromRedisDial), -return nil, fmt.Errorf("%s", ErrMsgFromRedisDial) +return nil, errors.New(ErrMsgFromRedisDial)These changes maintain functionality while being more concise and consistent with Go's error handling best practices.
Also applies to: 464-464, 521-522, 526-526, 548-549, 552-552
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
- Dockerfile (2 hunks)
- README.MD (8 hunks)
- go.mod (2 hunks)
- pkg/utl/rediscache/redis_test.go (3 hunks)
- pkg/utl/rediscache/service_test.go (22 hunks)
- pkg/utl/resultwrapper/error.go (3 hunks)
- pkg/utl/resultwrapper/error_test.go (12 hunks)
- pkg/utl/zaplog/zaplog_test.go (1 hunks)
- resolver/auth_mutations.resolvers_test.go (15 hunks)
- scripts/setup-pre-commit.sh (1 hunks)
- tools.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tools.go
🧰 Additional context used
🪛 LanguageTool
README.MD
[uncategorized] ~81-~81: Possible missing comma found.
Context: ...boiler-psql@latestFor Go 1.18 and above install the sql-migrate usingbash ...(AI_HYDRA_LEO_MISSING_COMMA)
[style] ~97-~97: Consider shortening or rephrasing this to strengthen your wording.
Context: ... the using docker-compose everytime you make changes to it # Setting up database (postgres) -...(MAKE_CHANGES)
[grammar] ~251-~251: Did you mean “be generated”?
Context: ...kg/api/api.go) ## Schema - Schema can generated or altered manually Take a look at t...(WILL_BASED_ON)
🔇 Additional comments (35)
scripts/setup-pre-commit.sh (1)
16-19:⚠️ Potential issueImprove go-commitlinter installation and git hook setup
While adding the go-commitlinter is a good addition for enforcing commit message standards, there are a few issues with the current implementation:
- The installed version (0.1.0) might be outdated. Consider using the latest version or at least a more recent one.
- The modification to the commit-msg hook is incomplete. Simply appending "go-commitlinter" to the file won't make it run.
Here's a suggested improvement:
-go install github.com/masahiro331/[email protected] +go install github.com/masahiro331/go-commitlinter@latest -touch .git/hooks/commit-msg -echo "go-commitlinter" >> .git/hooks/commit-msg -chmod 755 .git/hooks/commit-msg +HOOK_FILE=".git/hooks/commit-msg" +echo '#!/bin/sh' > "$HOOK_FILE" +echo 'go-commitlinter $1' >> "$HOOK_FILE" +chmod 755 "$HOOK_FILE"This change will:
- Install the latest version of go-commitlinter.
- Create a proper shell script in the commit-msg hook that actually runs the commitlinter.
To ensure the hook is set up correctly, you can run:
This should display the content of the commit-msg hook, which should now include the actual command to run go-commitlinter.
Dockerfile (3)
5-9: Excellent optimization of dependency management!The separation of copying
go.modandgo.sumfiles before runninggo mod downloadis a great improvement. This change leverages Docker's layer caching mechanism, potentially speeding up subsequent builds when dependencies haven't changed. It's a best practice for Dockerfiles in Go projects.
11-12: Good sequencing of copy operations.Copying the entire project after handling dependencies is the correct approach. This ensures that changes to the application code don't invalidate the cached dependency layer. The added comment also improves the Dockerfile's readability.
Line range hint
1-67: Overall, good improvements to the Dockerfile structure and build process.The changes made to this Dockerfile represent significant improvements in the build process for the Go application:
- The separation of dependency management (copying go.mod and go.sum first) optimizes caching.
- The logical sequencing of copying operations (dependencies, then the whole project) is correct.
- Consolidating build commands reduces the number of layers in the final image.
These changes should result in faster build times, especially when dependencies don't change, and a potentially smaller final image. The structure now better follows Docker best practices for Go applications.
To further enhance the Dockerfile:
- Consider the necessity of running the seeder during the build process.
- Evaluate the trade-offs between a single consolidated build command and separate build commands for each executable.
Great work on improving the Docker build process!
pkg/utl/rediscache/redis_test.go (7)
40-42: LGTM: Correct package update for DialOptionThe change from
redis.DialOptiontoredigo.DialOptionis correct and consistent with the migration to theredigopackage.
44-46: LGTM: Consistent package update for DialOptionThe change from
redis.DialOptiontoredigo.DialOptionis correct and consistent with the previous change and the overall migration to theredigopackage.
94-96: LGTM: Consistent package update for DialOptionThe change from
redis.DialOptiontoredigo.DialOptionis correct and consistent with previous changes and the overall migration to theredigopackage.
102-103: LGTM: Consistent package update for DialOptionThe change from
redis.DialOptiontoredigo.DialOptionis correct and consistent with previous changes and the overall migration to theredigopackage.
152-154: LGTM: Consistent package update for DialOptionThe change from
redis.DialOptiontoredigo.DialOptionis correct and consistent with previous changes and the overall migration to theredigopackage.
Line range hint
1-172: Overall assessment: Consistent package migration with one point to verifyThe changes in this file consistently update the package references from
redistoredigo, which aligns with the apparent objective of migrating to theredigopackage. All modifications to function signatures and type references are correct and consistent.There is one point that requires verification:
- The introduction and definition of the
ErrMsgMarshalconstant (see previous comment).Once this point is addressed, the changes in this file appear to be complete and correct.
108-108: Verify the ErrMsgMarshal constantThe
ErrMsgMarshalconstant is used here, but its definition is not visible in this file. Please ensure that this constant is properly defined and imported.✅ Verification successful
ErrMsgMarshal is properly defined and accessible
The
ErrMsgMarshalvariable is defined inpkg/utl/rediscache/service_test.goand is accessible inpkg/utl/rediscache/redis_test.go. No issues found regarding the definition and import.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and usage of ErrMsgMarshal constant # Test 1: Search for the definition of ErrMsgMarshal echo "Searching for ErrMsgMarshal definition:" rg --type go 'const.*ErrMsgMarshal' # Test 2: Check for imports that might include ErrMsgMarshal echo "Checking for relevant imports in redis_test.go:" rg --type go '^import \(' pkg/utl/rediscache/redis_test.go -A 10 # Test 3: Search for other usages of ErrMsgMarshal echo "Searching for other usages of ErrMsgMarshal:" rg --type go 'ErrMsgMarshal'Length of output: 789
go.mod (3)
33-33: LGTM: strmangle dependency update to v0.0.6The update of
github.com/volatiletech/strmanglefrom v0.0.4 to v0.0.6 is likely to include backwards-compatible changes. However, it's advisable to check the changelog for any breaking changes or new features that might affect the project.To verify the impact of this update, run the following command:
#!/bin/bash # Description: Check for usage of strmangle package and potential breaking changes # Test: Find all usages of the strmangle package rg --type go 'github\.com/volatiletech/strmangle' # Note: Manual review of the strmangle changelog is required to identify any breaking changes echo "Please manually review the changelog for github.com/volatiletech/strmangle between v0.0.4 and v0.0.6"
Line range hint
1-33: LGTM: Removal of older sqlboiler dependencyThe removal of
github.com/volatiletech/sqlboiler v3.7.1+incompatiblewhile keepinggithub.com/volatiletech/sqlboiler/v4 v4.11.0is a good practice to avoid potential conflicts. However, it's crucial to ensure that all references to the old package have been updated or removed throughout the project.To verify that all references to the old package have been updated, run the following command:
#!/bin/bash # Description: Check for any remaining references to the old sqlboiler package # Test: Search for any imports of the old sqlboiler package rg --type go 'github\.com/volatiletech/sqlboiler[^/]' # Test: Search for any uses of the old sqlboiler package name rg --type go '\bsqlboiler\.' # Note: If any results are found, they may need to be updated to use the new package
3-3: LGTM: Go version update to 1.22.0The update to a more specific Go version (1.22.0) is good for reproducibility. However, ensure that this version is compatible with all project dependencies and requirements.
To verify compatibility, run the following commands:
pkg/utl/resultwrapper/error.go (4)
81-81: Improved error message formattingThe change from
fmt.Errorf(errMessage)tofmt.Errorf("%s", errMessage)is a good practice. It ensures that the error message is treated as a plain string, preventing any potential formatting issues if the error message contains format verbs (like %s, %d, etc.). This change improves the robustness of the error handling.
155-155: Consistent error formatting in GraphQL errorsThis change from
gqlerror.Errorf(errMsg)togqlerror.Errorf("%s", errMsg)is consistent with the earlier modification in theWrapperFromMessagefunction. It applies the same best practice of using an explicit format specifier to GraphQL error handling. This ensures that all error messages, regardless of their source, are handled consistently throughout the application.
199-199: Consistent error formatting across wrapper functionsThis change from
fmt.Errorf(errMessage)tofmt.Errorf("%s", errMessage)in theResolverWrapperFromMessagefunction maintains consistency with the earlier modifications in this file. It's good to see that this best practice for error formatting is being applied uniformly across different wrapper functions, enhancing the overall robustness and maintainability of the error handling system.
Line range hint
81-199: Overall improvement in error handling consistencyThe changes made to this file demonstrate a systematic approach to improving error handling. By consistently using
fmt.Errorf("%s", errMessage)and its equivalent in GraphQL errors, the code now handles error messages more robustly across different contexts (standard errors, GraphQL errors, and resolver errors). This uniformity not only prevents potential formatting issues but also enhances the maintainability of the codebase. Good job on applying this best practice consistently throughout the file.pkg/utl/resultwrapper/error_test.go (8)
124-124: LGTM: Improved error message formattingThe change from
fmt.Errorf(ErrMsgJSON)tofmt.Errorf("%s", ErrMsgJSON)improves code clarity by explicitly formatting the error message as a string. This is a good practice and enhances consistency throughout the codebase.
135-135: LGTM: Consistent error message formattingThe change from
fmt.Errorf(ErrMsgJSON)tofmt.Errorf("%s", ErrMsgJSON)maintains consistency with the previous change and improves code clarity. This is a good practice that enhances the overall quality of the codebase.
165-165: LGTM: Consistent error formatting in test caseThe change from
fmt.Errorf(ErrMsg)tofmt.Errorf("%s", ErrMsg)in the test case maintains consistency with the previous changes and improves code clarity. This ensures that the error message is explicitly formatted as a string, which is a good practice in test cases as well.
201-201: LGTM: Improved assertion consistencyThe change from
assert.Equal(t, err, fmt.Errorf(tt.args.err))toassert.Equal(t, err, fmt.Errorf("%s", tt.args.err))in the assertion improves the consistency of error formatting in the test case. This ensures that the expected error is formatted in the same way as the actual error, enhancing the reliability of the test.
223-223: LGTM: Consistent error formatting in test caseThe change from
fmt.Errorf(errorStr)tofmt.Errorf("%s", errorStr)in the test case maintains consistency with the previous changes and improves code clarity. This ensures that the error message is explicitly formatted as a string, which is a good practice in test cases.
259-259: LGTM: Improved assertion consistencyThe change from
assert.Equal(t, err, fmt.Errorf(tt.args.err))toassert.Equal(t, err, fmt.Errorf("%s", tt.args.err))in the assertion improves the consistency of error formatting in the test case. This ensures that the expected error is formatted in the same way as the actual error, enhancing the reliability of the test.
281-281: LGTM: Consistent error formatting in test caseThe change from
fmt.Errorf(errorStr)tofmt.Errorf("%s", errorStr)in the test case maintains consistency with the previous changes and improves code clarity. This ensures that the error message is explicitly formatted as a string, which is a good practice in test cases.
594-594: LGTM: Improved assertion consistency and overall error handlingThe change from
assert.Equal(t, fmt.Errorf(errorMessage), err)toassert.Equal(t, fmt.Errorf("%s", errorMessage), err)in the assertion improves the consistency of error formatting in the test case. This ensures that the expected error is formatted in the same way as the actual error, enhancing the reliability of the test.Overall, these changes throughout the file improve the consistency and clarity of error handling in the test cases. By explicitly formatting error messages as strings using
fmt.Errorf("%s", ...), the code becomes more robust and less prone to potential formatting issues. This is a good practice that enhances the overall quality and maintainability of the test suite.pkg/utl/rediscache/service_test.go (3)
87-87: LGTM: Improved type specificity forinitfunctionThe change from
*Patchesto*gomonkey.Patchesenhances type safety and clarity. This is a good practice that helps prevent potential errors and improves code readability.
124-125: LGTM: Consistent use of*gomonkey.PatchesThe change from
*Patchesto*gomonkey.Patchesin theinitfunction signature is consistent with the earlier change in the struct definition. This improves type consistency throughout the file.
Line range hint
1-564: Overall assessment: Improved type safety with room for minor optimizationsThe changes in this file consistently improve type safety by using more specific types (
*gomonkey.Patches) and maintain a consistent error handling pattern. These modifications enhance code quality and reduce potential for errors.However, there's an opportunity to further improve the code by simplifying error creation. Consider replacing all instances of
fmt.Errorf("%s", errorString)witherrors.New(errorString)throughout the file. This change would make the code more idiomatic and slightly more efficient.Great job on the consistent improvements! The suggested optimizations are minor and don't detract from the overall quality enhancement achieved by these changes.
resolver/auth_mutations.resolvers_test.go (6)
86-90: Improved error message formattingThe changes in this segment standardize the error message formatting by using
fmt.Errorf("%s", ...). This approach improves code consistency and readability across error cases.
Line range hint
103-120: Consistent error message formattingThe changes in this segment continue the standardization of error message formatting using
fmt.Errorf("%s", ...). This consistency across different test cases improves the overall code quality and maintainability.
Line range hint
167-191: Consistent error formatting across test casesThe changes in this segment further demonstrate the consistent application of standardized error message formatting using
fmt.Errorf("%s", ...). This uniformity across various test scenarios enhances code readability and maintainability.
Line range hint
406-478: Consistent error formatting in password change test casesThe changes in this segment demonstrate the continued application of standardized error message formatting using
fmt.Errorf("%s", ...)across various password change test scenarios. This consistency enhances the overall readability and maintainability of the test suite.
Line range hint
595-619: Consistent error formatting in refresh token test casesThe changes in this segment extend the standardization of error message formatting using
fmt.Errorf("%s", ...)to the refresh token test scenarios. This consistency across different types of test cases further improves the overall code quality and maintainability of the test suite.
Line range hint
632-660: Consistent error formatting across all test casesThe changes in this final segment complete the standardization of error message formatting using
fmt.Errorf("%s", ...)across all test cases, including the remaining refresh token scenarios. This comprehensive update ensures consistency throughout the entire test suite, significantly improving code readability and maintainability.Overall, these changes represent a systematic improvement in error handling and formatting across the entire file, which will make future maintenance and debugging easier.
|
Closing the PR, because Raising PR from |
Ticket Link
Related Links
Description
Steps to Reproduce / Test
Request
Response
Summary by CodeRabbit
Release Notes
New Features
go-commitlintertool to improve commit message standards.Bug Fixes
Documentation
Chores