Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

Remove tkgconfigpaths.DefaultTKGBOMRepo and tkgconfigpaths.DefaultTKGCompatibilityPath dependency from pkg/v1/config #3264

Merged

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Sep 2, 2022

What this PR does / why we need it

  • This PR updates the the pkg/v1/config module to not rely on tkgconfigpaths package.
  • Currently, pkg/v1/config was using to fetch DefaultTKGBOMRepo and DefaultTKGCompatibilityPath information from tkgconfigpaths. Considering we are do not want to store plugins specific configuration into the Tanzu Config file we can remove this dependency and we do not need to dynamically set this information in the tkg client config.
  • As part of the PR, I am removing this dependency and hard-coding values that Core CLI will be configuring for clientOptions.cli. bomRepo and clientOptions.cli. compatibilityFilePath to default TKG production registry to make sure existing TKG plugins continue to work as it is.

Which issue(s) this PR fixes

Related to #3149

Describe testing done for PR

Release note

Remove configuring `clientOptions.cli.bomRepo` and `clientOptions.cli.compatibilityFilePath` from Tanzu-Config and use hard-coded values to support legacy plugins. This configuration is deprecated and will be removed from future version.

Additional information

Special notes for your reviewer

@anujc25 anujc25 requested review from a team as code owners September 2, 2022 23:56
@github-actions
Copy link

github-actions bot commented Sep 3, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/3264/20220903000355/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #3264 (2462336) into main (62e02ff) will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##             main    #3264   +/-   ##
=======================================
  Coverage   45.82%   45.82%           
=======================================
  Files         361      361           
  Lines       37640    37653   +13     
=======================================
+ Hits        17250    17256    +6     
- Misses      18818    18824    +6     
- Partials     1572     1573    +1     
Impacted Files Coverage Δ
pkg/v1/config/bom.go 14.28% <0.00%> (ø)
pkg/v1/tkg/tkgconfigupdater/ensure.go 14.98% <0.00%> (+0.27%) ⬆️
pkg/v1/tkg/managementcomponents/helper.go 92.10% <100.00%> (+0.21%) ⬆️
addons/controllers/machine_controller.go 65.65% <0.00%> (-3.04%) ⬇️
pkg/v1/config/defaults.go 41.50% <0.00%> (-0.80%) ⬇️
pkg/v1/cli/artifact/http.go 0.00% <0.00%> (ø)
pkg/v1/cli/command/core/discovery_source.go 41.86% <0.00%> (+1.95%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.
We should add to the release notes that these settings will be removed.

Also, we should find a place to explain (I think a .md in-tree that walks through how BOMs are processed should be the one to cover this) that if the default compat/bom locations are not what is required for the CLI/plugins, that env vars should continue to be used to override them.

@anujc25
Copy link
Contributor Author

anujc25 commented Sep 6, 2022

We should add to the release notes that these settings will be removed.
Updated the Release-Notes.

Also, we should find a place to explain (I think a .md in-tree that walks through how BOMs are processed should be the one to cover this) that if the default compat/bom locations are not what is required for the CLI/plugins, that env vars should continue to be used to override them.

I will file separate issue to track the documentation on how BoMs are processed and the changes regarding default compat/bom locations are not what is required for CLI/plugins.

@anujc25 anujc25 added area/plugin ok-to-merge PRs should be labelled with this before merging area/core-cli labels Sep 6, 2022
@anujc25 anujc25 merged commit 9057a8e into vmware-tanzu:main Sep 6, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area/core-cli area/plugin cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants