vmware / terraform-provider-tanzu-mission-control

Terraform provider to manage resources of Tanzu Mission Control
Mozilla Public License 2.0
38 stars 31 forks source link

Migrate inline_values to path_to_inline_values for package install #386

Closed ishangupta-ds closed 4 months ago

ishangupta-ds commented 5 months ago
  1. What this PR does / why we need it:

Fully Support for Tanzu Package by migrating inline_values to path_to_inline_values for package install.

Right now, when installing the Tanzu Package, inline_values can only be specified as String or float. However, most of the Tanzu Package has structural parameters rather than String or Float. Therefore, most Tanzu Packages cannot be deployed with the TMC Provider.

This PR also fixes broken tests for some packaging feature APIs. Namely - Packages, Helm Feature and Helm Release.

  1. Which issue(s) this PR fixes

Fixes #362

  1. Additional information

Since values is written in YAML, YAML support is desirable.

  1. Special notes for your reviewer

Enhancement / Bug fix PR.

Screenshots:

Package Install

Screenshot 2024-03-29 at 4 19 26 PM

Packages

Screenshot 2024-03-28 at 11 10 22 AM

Helm Feature

Screenshot 2024-03-29 at 9 44 35 AM

Helm Release

Screenshot 2024-03-29 at 10 42 58 AM
codecov-commenter commented 5 months ago

Codecov Report

Attention: Patch coverage is 11.27451% with 181 lines in your changes are missing coverage. Please review.

Project coverage is 24.60%. Comparing base (0dfb827) to head (c0dd7f0). Report is 2 commits behind head on main.

Files Patch % Lines
internal/helper/yaml.go 0.00% 58 Missing :warning:
...nternal/resources/tanzupackageinstall/spec/spec.go 0.00% 38 Missing :warning:
.../tanzupackageinstall/package_install_datasource.go 0.00% 34 Missing :warning:
...es/tanzupackageinstall/package_install_resource.go 0.00% 26 Missing :warning:
...esources/tanzupackageinstall/spec/cluster_scope.go 25.80% 21 Missing and 2 partials :warning:
...nal/resources/helmrelease/resource_helm_release.go 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #386 +/- ## ========================================== - Coverage 25.23% 24.60% -0.64% ========================================== Files 195 196 +1 Lines 16908 17079 +171 ========================================== - Hits 4267 4202 -65 - Misses 12423 12678 +255 + Partials 218 199 -19 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ishangupta-ds commented 5 months ago

...es/tanzupackageinstall/package_install_resource.go - has acceptance tests

...esources/tanzupackageinstall/spec/cluster_scope.go - we don't have construct tests across the provider

...nternal/resources/tanzupackageinstall/spec/spec.go - we don't have construct tests across the provider

...nal/resources/helmrelease/resource_helm_release.go - has acceptance tests

internal/helper/yaml.go - let me know if tests are needed for this file, since it's a simple file reading opration.

ishangupta-ds commented 5 months ago

Looking at the changes, see text & label changes for many resources beyond PackageInstall. Suggest that appropriate owners of said resources/features be looped for approval - to ensure there isn't any loss of information or intent.

Removed all unrelated changes.

ishangupta-ds commented 4 months ago

Migrating inline_values to path_to_inline_values seems like a breaking change, to me. Due to which existing customer Terraform scripts & states might break once these proposed changes are merged.

Would suggest not removing inline_values completely instead adding as addition field path_to_inline_values to package install; so that existing TF scripts & states in use aren't broken. And future usage can be adapt to use recommended path_to_inline_values.

Please note that final say here is with the feature team. So if FT feel the need to make such breaking changes, please go ahead and confirm intent to handle all customer tickets or feedback that the change brings.

Sure.

ishangupta-ds commented 4 months ago

@AkbaraliShaikh / @ramya-bangera / @vmw-vjn / @ankitsny Please take a look again.

ramya-bangera commented 4 months ago

@ishangupta-ds - You plan to merge this to main now? or it should go to the feature branch for tap saas?

ishangupta-ds commented 4 months ago

@ramya-bangera these bug fixes / changes are for already released SaaS features via Terraform, not related to the TAP enablement.

ishangupta-ds commented 4 months ago

Please take a look again. @AkbaraliShaikh / @ramya-bangera