Remove kubectl template dependency with simplified implementation #4807
Conversation
WalkthroughThis pull request focuses on removing internationalization (i18n) dependencies and simplifying template handling across multiple CLI command files in the Bacalhau project. The changes primarily involve removing imports from Kubernetes' Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Templates as Local Templates Package
participant UserInterface as User Interface
CLI->>Templates: Get Long Description
Templates-->>CLI: Normalized Description
CLI->>Templates: Get Examples
Templates-->>CLI: Formatted Examples
CLI->>UserInterface: Display Help Text
Possibly related PRs
Poem
Finishing Touches
🪧 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 comments (1)
go.mod (1)
Critical: Update golang.org/x/crypto package to fix authorization bypass vulnerability
The golang.org/x/crypto package (currently at v0.28.0) has a critical security vulnerability that could allow authorization bypass through misuse of ServerConfig.PublicKeyCallback. Update to at least version v0.31.0 to patch this issue.
🔗 Analysis chain
Line range hint
1-300: Verify security status of retained dependencies.Please ensure that all retained dependencies are using secure versions.
Let's check for any security advisories:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for security advisories for retained dependencies # Check heredoc package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/MakeNowJust/heredoc") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check apimachinery package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "k8s.io/apimachinery") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1654
Script:
#!/bin/bash # Check security advisories for security-critical dependencies # Check JWT package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/golang-jwt/jwt/v5") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check crypto package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "golang.org/x/crypto") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }' # Check libp2p package gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/libp2p/go-libp2p") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 3516
🧹 Nitpick comments (3)
cmd/util/templates/normalizers.go (1)
50-63: Consider optimizing string handling in indent methodThe current implementation could be more efficient by using strings.Builder to avoid multiple string concatenations.
Here's an optimized version:
func (s normalizer) indent() normalizer { - var indentedLines []string + var builder strings.Builder for _, line := range strings.Split(s.string, "\n") { trimmed := strings.TrimSpace(line) if trimmed != "" { - indented := Indentation + trimmed - indentedLines = append(indentedLines, indented) + builder.WriteString(Indentation) + builder.WriteString(trimmed) } else { - indentedLines = append(indentedLines, "") + builder.WriteString("") } + builder.WriteString("\n") } - s.string = strings.Join(indentedLines, "\n") + // Remove the trailing newline if the original string didn't end with one + result := builder.String() + if !strings.HasSuffix(s.string, "\n") { + result = strings.TrimSuffix(result, "\n") + } + s.string = result return s }cmd/util/templates/normalizers_test.go (1)
112-172: Consider adding boundary test cases for normalizer methods.While the current test cases are good, consider adding tests for:
- Maximum indentation levels
- Very long lines
- Special characters in heredoc
cmd/cli/job/stop.go (1)
35-43: Consider using constants for status messages.The status messages could be defined as constants to ensure consistency across the codebase and make updates easier to manage.
-var ( - checkingJobStatusMessage = "Checking job status" - connectingMessage = "Connecting to network" - gettingJobMessage = "Verifying job state" - stoppingJobMessage = "Stopping job" -) +const ( + checkingJobStatusMessage = "Checking job status" + connectingMessage = "Connecting to network" + gettingJobMessage = "Verifying job state" + stoppingJobMessage = "Stopping job" +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
cmd/cli/config/set.go(2 hunks)cmd/cli/devstack/devstack.go(3 hunks)cmd/cli/docker/docker_run.go(3 hunks)cmd/cli/job/describe.go(3 hunks)cmd/cli/job/executions.go(2 hunks)cmd/cli/job/get.go(2 hunks)cmd/cli/job/history.go(3 hunks)cmd/cli/job/list.go(2 hunks)cmd/cli/job/logs.go(2 hunks)cmd/cli/job/run.go(2 hunks)cmd/cli/job/stop.go(1 hunks)cmd/cli/job/validate.go(1 hunks)cmd/cli/serve/serve.go(3 hunks)cmd/cli/wasm/wasm_run.go(2 hunks)cmd/util/templates/normalizers.go(1 hunks)cmd/util/templates/normalizers_test.go(1 hunks)go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- cmd/cli/wasm/wasm_run.go
- cmd/cli/job/logs.go
- cmd/cli/docker/docker_run.go
- cmd/cli/job/get.go
🔇 Additional comments (20)
cmd/util/templates/normalizers.go (3)
1-10: Well-structured package documentation and imports!Good job on properly crediting the original kubectl templates package and keeping the imports minimal.
14-23: Clean and efficient implementation!The method chaining approach makes the transformation steps clear and maintainable. Good handling of empty strings for efficiency.
25-34: Consistent implementation pattern!The Examples function follows the same clean pattern as LongDesc while applying appropriate transformations for example text.
cmd/cli/job/validate.go (1)
14-18: Clean migration from kubectl templates!The removal of i18n.T() wrapper and migration to the new templates package is done correctly. The documentation strings remain clear and well-formatted.
Also applies to: 21-24
cmd/cli/job/history.go (1)
23-25: Consistent template migration!The documentation strings are well-maintained while removing the i18n dependency. The examples are clear and properly formatted.
Also applies to: 27-36
cmd/cli/config/set.go (1)
21-25: Clean template migration with clear examples!The migration to the new templates package is done correctly while maintaining clear and helpful examples.
cmd/util/templates/normalizers_test.go (2)
10-60: LGTM! Comprehensive test coverage for LongDesc function.The test cases effectively cover various scenarios including empty strings, single line inputs, multi-line inputs with indentation, and whitespace handling.
62-110: LGTM! Well-structured test cases for Examples function.The test cases thoroughly validate the Examples function's behavior with different input formats and indentation scenarios.
cmd/cli/job/stop.go (1)
20-31: LGTM! Clean transition from i18n to direct string literals.The removal of i18n dependency simplifies the code while maintaining readability.
cmd/cli/job/list.go (1)
30-40: LGTM! Clean documentation strings.The removal of i18n wrapper maintains clear and readable documentation while reducing dependencies.
cmd/cli/job/run.go (1)
23-27: LGTM! Clear and concise long description.The long description effectively communicates the command's purpose and supported formats.
cmd/cli/job/executions.go (2)
13-13: LGTM: Import change aligns with PR objective.The change from kubectl's template package to local implementation is consistent with the PR's goal of removing the kubectl dependency.
29-31: LGTM: Template usage properly migrated.The migration from kubectl's i18n.T() to local templates package is correctly implemented, maintaining the same functionality while removing the dependency.
Also applies to: 33-36
cmd/cli/job/describe.go (2)
18-18: LGTM: Import change aligns with PR objective.The change from kubectl's template package to local implementation is consistent with the PR's goal of removing the kubectl dependency.
27-30: LGTM: Template usage properly migrated.The migration from kubectl's i18n.T() to local templates package is correctly implemented, maintaining the same functionality while removing the dependency.
Also applies to: 31-40
cmd/cli/devstack/devstack.go (2)
19-19: LGTM: Import change aligns with PR objective.The change from kubectl's template package to local implementation is consistent with the PR's goal of removing the kubectl dependency.
29-31: LGTM: Template usage properly migrated.The migration from kubectl's i18n.T() to local templates package is correctly implemented, maintaining the same functionality while removing the dependency.
Also applies to: 34-46
cmd/cli/serve/serve.go (2)
13-13: LGTM: Import change aligns with PR objective.The change from kubectl's template package to local implementation is consistent with the PR's goal of removing the kubectl dependency.
32-34: LGTM: Template usage properly migrated.The migration from kubectl's i18n.T() to local templates package is correctly implemented, maintaining the same functionality while removing the dependency.
Also applies to: 36-48
go.mod (1)
165-165: LGTM! Heredoc dependency retained as required.The retention of
github.com/MakeNowJust/heredocaligns with the PR objectives to preserve heredoc support for multiline strings.
| runExample = templates.Examples(` | ||
| # Run a job using the data in job.yaml | ||
| bacalhau job run ./job.yaml | ||
|
|
||
| # Run a new job from an already executed job | ||
| bacalhau job describe 6e51df50 | bacalhau job run | ||
|
|
||
| # Download the | ||
| `)) | ||
| `) |
There was a problem hiding this comment.
Fix incomplete example documentation.
The example documentation is cut off mid-sentence: "# Download the". Please complete the example to provide clear guidance to users.
This PR removes the dependency on kubectl's template package by providing a simplified implementation for CLI help text formatting. Binary size reduced from
92Mto81M. The new implementation:The result is a lighter, more focused package that handles just what we need for CLI help formatting while removing a heavy dependency.
Original inspiration from kubectl is credited in the package documentation.
Summary by CodeRabbit
Refactor
Chores
go.modNew Features
templatesutility package for CLI text formatting