-
Notifications
You must be signed in to change notification settings - Fork 6
Add KRM exec and Docker functions support #12
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
base: main
Are you sure you want to change the base?
Conversation
@aabouzaid the new PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive KRM (Kubernetes Resource Model) exec and Docker functions support to Secretize, modernizing the tool to work with current Kustomize versions while maintaining backward compatibility with the legacy plugin system.
Key changes:
- Implements KRM function support with both exec and containerized modes
- Adds comprehensive examples and documentation for all usage patterns
- Fixes string literal parsing bug for key-value pairs containing equals signs
- Updates dependencies and Go version to support modern Kubernetes tooling
Reviewed Changes
Copilot reviewed 34 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
cmd/secretize/main.go |
Adds KRM function processor and dual-mode execution support |
pkg/generator/generator.go |
Fixes literal parsing bug and import organization |
pkg/generator/krm_integration_test.go |
Comprehensive test suite for KRM function integration |
go.mod |
Updates Go version and adds KRM dependencies |
Dockerfile |
New containerized KRM function support |
examples/ |
Extensive examples for legacy, exec, and Docker KRM functions |
README.md |
Updated documentation with KRM function usage patterns |
Comments suppressed due to low confidence (2)
pkg/generator/krm_integration_test.go:15
- The test doesn't verify that environment variables are properly cleaned up if the test fails before the defer statement executes. Consider using t.Setenv() which automatically handles cleanup.
os.Setenv("TEST_LITERAL", "literal-value")
pkg/generator/krm_integration_test.go:157
- The test case for input "simple" expects both key and value to be "simple", but this doesn't match the actual parsing logic which should handle cases without '=' differently.
{"simple", "simple", "simple"},
l.Key = res[0] | ||
l.Value = res[1] | ||
// Split on the first "=" only | ||
idx := strings.Index(stringLiteral, "=") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code doesn't handle the case where no '=' is found in the string literal. If strings.Index returns -1, the subsequent slice operations will cause a panic or incorrect behavior.
idx := strings.Index(stringLiteral, "=") | |
idx := strings.Index(stringLiteral, "=") | |
if idx == -1 { | |
return nil | |
} |
Copilot uses AI. Check for mistakes.
|
||
// isKRMFunction checks if stdin has content (indicating KRM function mode) | ||
func isKRMFunction() bool { | ||
stat, _ := os.Stdin.Stat() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Error from os.Stdin.Stat() is ignored. While this might be intentional for detecting stdin availability, consider handling potential errors or adding a comment explaining why the error is safely ignored.
stat, _ := os.Stdin.Stat() | |
stat, err := os.Stdin.Stat() | |
if err != nil { | |
log.Warnf("Failed to stat stdin: %v", err) | |
return false | |
} |
Copilot uses AI. Check for mistakes.
Add KRM exec and Docker functions support