feat: add environment file check pass whitelist#913
feat: add environment file check pass whitelist#913hwbrzzl merged 3 commits intogoravel:masterfrom go-projects:env
Conversation
WalkthroughThe changes update how application key initialization and command-line arguments are processed. In Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24.0) ✨ Finishing Touches
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 (
|
|
Ready Review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #913 +/- ##
==========================================
- Coverage 68.86% 68.83% -0.03%
==========================================
Files 154 154
Lines 10196 10190 -6
==========================================
- Hits 7021 7014 -7
- Misses 2852 2854 +2
+ Partials 323 322 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/application.go(1 hunks)foundation/application.go(1 hunks)support/constant.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
foundation/application.go
[warning] 281-282: foundation/application.go#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: foundation/application.go#L284
Added line #L284 was not covered by tests
🔇 Additional comments (2)
support/constant.go (1)
11-22: LGTM! The variable reorganization improves code readability.The changes improve code organization by:
- Grouping related environment variables together.
- Introducing a whitelist mechanism for environment verification bypass.
config/application.go (1)
38-52: LGTM! The condition change maintains the same logic while using the new whitelist mechanism.The error messages provide clear guidance on how to initialize the APP_KEY.
foundation/application.go
Outdated
| support.EnvVerifyWhitelist = append(support.EnvVerifyWhitelist, arg) | ||
| } | ||
| if arg == "env:decrypt" { | ||
| support.EnvVerifyWhitelist = append(support.EnvVerifyWhitelist, arg) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for the new whitelist entries.
The static analysis indicates that the new lines adding commands to EnvVerifyWhitelist are not covered by tests. Please add test cases to verify:
- The "key:generate" command is correctly added to the whitelist.
- The "env:decrypt" command is correctly added to the whitelist.
🏁 Script executed:
#!/bin/bash
# Description: Check for existing test files that might be suitable for adding the new test cases.
echo "Searching for test files related to command handling..."
fd -e go -x grep -l "key:generate\|env:decrypt" {}Length of output: 377
Action Required: Enhance Test Coverage for Whitelist Updates
Static analysis indicates that test coverage for the whitelist updates in foundation/application.go (lines 281-285) is incomplete. While the test file ./foundation/console/env_decrypt_command_test.go appears to cover the "env:decrypt" command, there is no dedicated test ensuring that the "key:generate" command is correctly added to EnvVerifyWhitelist.
Please address the following:
- Add a test case for "key:generate": Ensure that when the "key:generate" command is processed, it is correctly appended to
EnvVerifyWhitelist. - Review existing env:decrypt tests: Confirm that the tests in
./foundation/console/env_decrypt_command_test.goexplicitly verify that the whitelist update for "env:decrypt" is functioning as expected.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 281-282: foundation/application.go#L281-L282
Added lines #L281 - L282 were not covered by tests
[warning] 284-284: foundation/application.go#L284
Added line #L284 was not covered by tests
foundation/application.go
Outdated
| if arg == "key:generate" { | ||
| support.IsKeyGenerateCommand = true | ||
| support.EnvVerifyWhitelist = append(support.EnvVerifyWhitelist, arg) | ||
| } | ||
| if arg == "env:decrypt" { |
There was a problem hiding this comment.
We can put key:generate and env:decrypt into a slice, then check if the slice contains arg here, if yes, set support.DontVerifyEnvFileExists=true. If there is another command, we can just add it to the slice.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
foundation/application.go (1)
351-358: Consider using a map for better performance.While the current implementation is clean and straightforward, using a map instead of a slice would provide O(1) lookup time, which could be beneficial if the whitelist grows larger.
Here's a suggested optimization:
-func InDontVerifyEnvFileWhitelist(item string) bool { - for _, eachItem := range support.DontVerifyEnvFileWhitelist { - if eachItem == item { - return true - } - } - return false -} +func InDontVerifyEnvFileWhitelist(item string) bool { + // Initialize map once during package initialization + whitelistMap := make(map[string]struct{}, len(support.DontVerifyEnvFileWhitelist)) + for _, cmd := range support.DontVerifyEnvFileWhitelist { + whitelistMap[cmd] = struct{}{} + } + _, exists := whitelistMap[item] + return exists +}Also, please add test cases to verify:
- The function returns true for whitelisted commands
- The function returns false for non-whitelisted commands
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 354-355: foundation/application.go#L354-L355
Added lines #L354 - L355 were not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/application.go(1 hunks)foundation/application.go(2 hunks)support/constant.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- config/application.go
- support/constant.go
🧰 Additional context used
🪛 GitHub Check: codecov/patch
foundation/application.go
[warning] 281-281: foundation/application.go#L281
Added line #L281 was not covered by tests
[warning] 354-355: foundation/application.go#L354-L355
Added lines #L354 - L355 were not covered by tests
foundation/application.go
Outdated
| if InDontVerifyEnvFileWhitelist(arg) { | ||
| support.DontVerifyEnvFileExists = true | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! But needs test coverage.
The implementation cleanly switches to a whitelist-based approach for commands that bypass environment file verification.
Please add test cases to verify:
- Commands in the whitelist correctly set
DontVerifyEnvFileExiststo true - Commands not in the whitelist don't affect
DontVerifyEnvFileExists
🏁 Script executed:
#!/bin/bash
# Description: Check for existing test files that might be suitable for adding the new test cases
echo "Searching for test files related to environment setup..."
fd -e go -x grep -l "setEnv\|DontVerifyEnvFileExists" {}Length of output: 257
LGTM – Implementation Approved, but Test Coverage is Incomplete
The changes to use the whitelist-based approach in the setEnv function are implemented cleanly and meet the PR objectives. However, our investigation shows that no dedicated test files exist to cover this new behavior. Please add test cases to verify that:
- Commands present in the whitelist correctly set
support.DontVerifyEnvFileExiststo true. - Commands not in the whitelist leave
support.DontVerifyEnvFileExistsunchanged.
Additional checks in a test file (e.g., a new application_test.go) should ensure robust regression coverage moving forward.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 281-281: foundation/application.go#L281
Added line #L281 was not covered by tests
support/constant.go
Outdated
| ) | ||
|
|
||
| var ( |
foundation/application.go
Outdated
| if InDontVerifyEnvFileWhitelist(arg) { | ||
| support.DontVerifyEnvFileExists = true | ||
| } |
There was a problem hiding this comment.
| if InDontVerifyEnvFileWhitelist(arg) { | |
| support.DontVerifyEnvFileExists = true | |
| } | |
| support.DontVerifyEnvFileExists = strings.Contains(support.DontVerifyEnvFileWhitelist, arg) |
There was a problem hiding this comment.
support.DontVerifyEnvFileExists = slices.Contains(support.DontVerifyEnvFileWhitelist, arg)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
support/constant.go (2)
18-19: Naming suggestion for improved clarityThe variable name
DontVerifyEnvFileExistsuses a negative construction which can be harder to understand in conditional statements. Consider renaming to a positive construction likeSkipEnvFileVerificationfor improved readability.- DontVerifyEnvFileExists = false + SkipEnvFileVerification = false
19-19: Add a comment explaining the whitelist purposeConsider adding a comment above the whitelist to explain why these specific commands are exempted from environment file verification. This will help future maintainers understand the rationale behind this feature.
+ // Commands that don't require environment file verification DontVerifyEnvFileWhitelist = []string{"key:generate", "env:decrypt"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
foundation/application.go(2 hunks)support/constant.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- foundation/application.go
⏰ Context from checks skipped due to timeout of 300000ms (1)
- GitHub Check: test / windows (1.23)
📑 Description
Summary by CodeRabbit
New Features
Refactor
✅ Checks