Conversation
…cent revisions
manusa
left a comment
There was a problem hiding this comment.
Pull Request Review: #353
Summary
This PR implements the helm history command as specified in issue #350. It adds the full stack implementation: Go native layer, JNA bridge, and Java command API. The implementation allows users to retrieve release history with options for max revisions, namespace, and kubeconfig.
Review Verdict
REQUEST_CHANGES
Findings
Critical Issues (Must Fix)
-
Missing Apache License header in
ReleaseHistory.javaAll source files in this project require the Apache License 2.0 header. The
ReleaseHistory.javafile is missing this header. Compare withRelease.javawhich has the proper license header.The file should start with:
/* * Copyright 2024 Marc Nuri * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software * distributed under the License is distributed on an "AS IS" BASIS, * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. * See the License for the specific language governing permissions and * limitations under the License. */
-
Go implementation should use
url.Valuespattern (native/internal/helm/history.go)The current implementation uses
fmt.Sprintfwithurl.QueryEscape:out.WriteString(fmt.Sprintf("revision=%d&updated=%s&status=%s&chart=%s&appVersion=%s&description=%s\n", rel.Version, url.QueryEscape(rel.Info.LastDeployed.Format(time.RFC1123Z)), ...
For consistency with the rest of the codebase, this should use
url.Valueslikelist.godoes:out := bytes.NewBuffer(make([]byte, 0)) for _, rel := range releases { values := make(url.Values) values.Set("revision", strconv.Itoa(rel.Version)) if tspb := rel.Info.LastDeployed; !tspb.IsZero() { values.Set("updated", tspb.Format(time.RFC1123Z)) } values.Set("status", rel.Info.Status.String()) values.Set("chart", formatChartname(rel.Chart)) values.Set("appVersion", formatAppVersion(rel.Chart)) values.Set("description", rel.Info.Description) _, _ = fmt.Fprintln(out, values.Encode()) }
This also addresses the need to use
formatAppVersion()helper which handles nil chart edge cases (per Helm issue #1347). -
Tests should be nested in
HelmKubernetesTestfor performanceAll tests using KinD container are grouped within
HelmKubernetesTestas nested classes. This avoids spinning up multiple KinD containers (expensive operation) across different test files.The
HelmHistoryTestclass should be refactored to become a nested classHistoryinsideHelmKubernetesTest, following the same pattern asHelmList,Uninstall,Upgrade, etc.I've updated issue #350 to reflect this testing pattern requirement.
Suggestions (Should Consider)
-
Test assertions could be more idiomatic
Multiple
assertThat()calls on the same object could be combined for better readability:// Current: assertThat(releaseHistories).hasSize(1); assertThat(releaseHistories.get(0).getRevision()).isEqualTo(1); assertThat(releaseHistories.get(0).getDescription()).containsIgnoringCase("Install complete"); // Suggested: assertThat(releaseHistories) .hasSize(1) .first() .returns(1, ReleaseHistory::getRevision) .extracting(ReleaseHistory::getDescription).asString() .containsIgnoringCase("Install complete");
This follows the pattern used elsewhere in
HelmKubernetesTest.
Security Concerns
None identified. The implementation properly handles kubeconfig and uses URL encoding for data serialization.
Testing Assessment
Coverage: Good
The test suite covers:
- ✅ Basic history after install
- ✅ History after upgrade (multiple revisions)
- ✅ Max filter functionality
- ✅ Namespace isolation
- ✅ KubeConfig file path
- ✅ KubeConfig contents
- ✅ Error case (non-existent release)
Overall Assessment
This is a well-structured PR that follows the existing patterns in the codebase. The implementation is clean and matches the API design specified in issue #350. The Go implementation correctly handles the manual Max filter (with appropriate documentation explaining why).
Three critical issues must be addressed before merge:
- Add the missing Apache License header to
ReleaseHistory.java - Refactor Go code to use
url.Valuespattern andformatAppVersion()helper - Move tests to be nested within
HelmKubernetesTest
After these fixes, the PR is ready to merge.
🤖 Generated with Claude Code
|
Thanks for the review. As soon as I have some time, I'll fix the issues. |
|
@manusa - The requested changes have been applied |
Closes #350
Summary
• Adds support for the
helm historycommand• Implements the full stack: Go native layer, JNA bridge, and Java command API
What's Changed
Go Implementation (
native/helm/history.go)History()function withHistoryOptionsstructaction.History.Run()doesn't honor the Max field)Java API
HistoryCommandwith fluent API builder patternReleaseHistorymodel to parse release history entriesHelm.history(String releaseName)static methodwithMax(),withNamespace(),withKubeConfig(),withKubeConfigContents()Tests
Test plan
go test ./...in native/)mvn clean install -Dquickly)mvn test -Dtest=HelmHistoryTest)Usage