-
Notifications
You must be signed in to change notification settings - Fork 103
Refactor pkg/cli/helm and add unit tests #9166
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
Conversation
6676432
to
8fd7152
Compare
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: ytimocin <[email protected]>
8fd7152
to
20d15a7
Compare
Unit Tests3 734 tests +8 3 731 ✅ +7 6m 32s ⏱️ -7s Results for commit 5bff917. ± Comparison against base commit 140fe93. This pull request removes 3 and adds 11 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9166 +/- ##
==========================================
+ Coverage 55.92% 56.35% +0.42%
==========================================
Files 604 605 +1
Lines 41467 41495 +28
==========================================
+ Hits 23192 23386 +194
+ Misses 16500 16308 -192
- Partials 1775 1801 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
Signed-off-by: willdavsmith <[email protected]>
This comment has been minimized.
This comment has been minimized.
⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: Will Smith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: willdavsmith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Signed-off-by: willdavsmith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
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 PR description should explain why these changes are being made, especially if there is no issue linked. The PR checklist should also be filled out.
pkg/cli/helm/helmaction.go
Outdated
|
||
// QueryRelease checks to see if a release is deployed to a namespace for a given kubecontext. | ||
// Returns a bool indicating if the release is deployed, the version of the release, and an error if one occurs. | ||
QueryRelease(kubeContext, namespace, releaseName string) (bool, string, error) |
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 parameters are in a different order than the implementation and the code that calls the function.
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.
good catch. looks like 2 wrongs do make a right
pkg/cli/helm/helmaction_test.go
Outdated
} | ||
|
||
err := AddValues(&helmChart, options) | ||
err := addArgsFromCLI(&helmChart, options) | ||
require.Equal(t, err, nil) |
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.
Nit: use require.NoError(t, err)
pkg/cli/helm/helmaction_test.go
Outdated
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
*/ | ||
|
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.
Is removing the copyright header intentional?
Signed-off-by: willdavsmith <[email protected]>
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
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.
🚢
Description
Overview
the pkg/cli/helm package has been part of the Radius project since the beginning of the project (~4yrs ago). This package has been left mostly un-unit tested and has been the source of a few bugs in the past. As part of the Radius upgrades features, we will be adding code here, so I decided to refactor this package to add unit tests and improve code clarity.
Copilot-generated summary
This pull request introduces significant refactoring and enhancements to the Helm integration for managing Radius and its dependencies (e.g., Contour) in Kubernetes. The changes include the introduction of a new Helm interface, improved error handling, and updates to test cases to align with the new implementation. Below is a summary of the most important changes grouped by theme.
Helm Interface Enhancements:
Helm.Interface
abstraction to encapsulate Helm operations such asInstallRadius
,UninstallRadius
,UpgradeRadius
, andCheckRadiusInstall
. This improves modularity and testability.Helm.Impl
struct that uses a Helm client to perform the actual operations, replacing direct function calls to Helm utilities.Refactoring of Installation and Uninstallation Logic:
installRadius
function with a newInstallRadius
method in theHelm.Interface
. The new method applies both Radius and Contour Helm charts with enhanced error handling. [1] [2]UninstallRadius
logic by delegating it to the newHelm.Interface
, making the code cleaner and more maintainable. [1] [2]Updates to Default Cluster Options:
ClusterOptions
struct to separateRadiusChartOptions
andContourChartOptions
, making the configuration more explicit. Added default values for new fields such asWait
andHostNetwork
.PopulateDefaultClusterOptions
function to support overriding theChartVersion
for Radius.Test Case Adjustments:
kubernetes_test.go
andinit_test.go
to reflect the newInstallRadius
andUninstallRadius
method signatures. Removed the return value fromInstallRadius
and adjusted expectations accordingly. [1] [2] [3]Code Cleanup:
ContourChartDefaultVersion
andqueryRelease
, as they were replaced by the new Helm interface methods. [1] [2]Type of change
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: