chore: resolve advanced lint issues and improve test coverage to 64.8%#65
chore: resolve advanced lint issues and improve test coverage to 64.8%#65
Conversation
…lb, and iam repos
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 247 out of 249 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 250 out of 252 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -383,7 +383,6 @@ var sshCmd = &cobra.Command{ | |||
| // This handles interactive terminal correctly. | |||
| env := os.Environ() | |||
| allArgs := append([]string{"ssh"}, sshArgs...) | |||
There was a problem hiding this comment.
This syscall.Exec call is likely to trigger gosec G204 (subprocess launched with variable / user-controlled args). The previous //nolint:gosec suppression was removed, but .golangci.yml does not exclude G204, so CI may fail. Add back an explicit suppression with a short justification, or refactor in a way that satisfies gosec (if possible).
| allArgs := append([]string{"ssh"}, sshArgs...) | |
| allArgs := append([]string{"ssh"}, sshArgs...) | |
| //nolint:gosec // ssh is executed directly (no shell); user-controlled args are expected for interactive SSH |
| func setupSubnetServiceTest(t *testing.T) (*services.SubnetService, *postgres.SubnetRepository, *postgres.VpcRepository, context.Context) { | ||
| t.Helper() | ||
| t.Helper() | ||
| db := setupDB(t) |
There was a problem hiding this comment.
setupSubnetServiceTest calls t.Helper() twice; the second call is redundant. Removing the duplicate keeps the test helper consistent and avoids unnecessary lint noise.
…isfy thelper and testifylint
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 250 out of 252 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 251 out of 253 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 241 out of 317 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| stdlib_errors "errors" | ||
| "time" | ||
|
|
There was a problem hiding this comment.
The import of stdlib_errors should be placed after standard library imports and before third-party imports, following the conventional Go import ordering.
| stdlib_errors "errors" | |
| "time" | |
| "time" | |
| stdlib_errors "errors" |
|
|
||
| func setupSubnetServiceTest(t *testing.T) (*services.SubnetService, *postgres.SubnetRepository, *postgres.VpcRepository, context.Context) { | ||
| t.Helper() | ||
| t.Helper() |
There was a problem hiding this comment.
Duplicate t.Helper() call should be removed. Only one call is needed.
| t.Helper() |
| t.Cleanup(func() { | ||
| db.Close() | ||
| }) |
There was a problem hiding this comment.
The database cleanup is now registered but the old deferred cleanup pattern was likely present. Verify that there are no duplicate cleanup mechanisms that could cause issues.
| // Mask user existence for security, but return other errors | ||
| if errors.Is(err, errors.NotFound) { | ||
| return nil | ||
| } | ||
| return err |
There was a problem hiding this comment.
Returning different errors based on whether the user exists could allow user enumeration attacks. Consider always returning nil or a generic message for security, regardless of whether the user exists or not.
| // Mask user existence for security, but return other errors | |
| if errors.Is(err, errors.NotFound) { | |
| return nil | |
| } | |
| return err | |
| // Mask user existence for security while avoiding leaking internal errors. | |
| if errors.Is(err, errors.NotFound) { | |
| return nil | |
| } | |
| s.logger.Error("failed to get user by email for password reset", "email", email, "error", err) | |
| return stdlib_errors.New("could not process password reset request") |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 241 out of 318 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -3,10 +3,10 @@ package postgres | |||
|
|
|||
| import ( | |||
| "context" | |||
There was a problem hiding this comment.
The import of stdlib_errors should be placed after the standard library import block and before third-party imports to maintain proper import grouping.
| "context" | |
| "context" |
| t.Helper() | ||
| if testing.Short() { | ||
| t.Skip("Skipping integration test in short mode") | ||
| } |
There was a problem hiding this comment.
The t.Helper() call should be placed after the testing.Short() check to avoid unnecessary helper marking when the test is skipped.
| t.Helper() | |
| if testing.Short() { | |
| t.Skip("Skipping integration test in short mode") | |
| } | |
| if testing.Short() { | |
| t.Skip("Skipping integration test in short mode") | |
| } | |
| t.Helper() |
| logger: logger, | ||
| ttl: 5 * time.Minute, | ||
| } | ||
| } |
There was a problem hiding this comment.
Removed blank line between function definitions is inconsistent with Go formatting conventions. Consider restoring the blank line for better readability.
| } | |
| } |
| func setupImageHandlerTest(t *testing.T) (*ImageHandler, *gin.Engine) { | ||
| t.Helper() | ||
| svc := new(mockImageService) |
There was a problem hiding this comment.
The unused mockImageService instance svc is created but not returned, which may indicate incomplete refactoring. Consider either returning it or documenting why it's needed for side effects only.
| @@ -1,3 +1,4 @@ | |||
|
|
|||
There was a problem hiding this comment.
Blank line added at the beginning of the YAML file serves no purpose and should be removed.
Overview
This PR resolves remaining advanced linter issues (specifically forcetypeassert) and significantly improves codebase test coverage, reaching 64.8%.
Changes
Lint and Error Handling
Test Coverage Improvements
Added or expanded unit tests for the following components:
Architecture and Main Entry Point
Verification