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

Removes requirement adddon configs match cluster name #3652

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

adduarte
Copy link

@adduarte adduarte commented Oct 13, 2022

allowing the name of addons config to be anything

controllers changed in this patch

  • Calico CNI
  • Antrea CNI
  • CSI
  • CPI
  • Kapp Controller

scenario logic:

  • no customization
    user does nothing
    addonsNS/config-template is cloned to clusterNS/{CLUSTER_NAME}-{pacakge-short-name}-package
    CLUSTER_NAME will be added the owner references of clusterNS/{CLUSTER_NAME}-{pacakge-short-name}-package
    data value secrets is created

  • customize based on template:
    create clusterNS/addonconfig withname clusterNS/{CLUSTER_NAMEW}-{package-short-name}-pacakge
    then
    clusterNS/{CLUSTER_NAME}-{pacakge-short-name}-package += addonsNS/config-template

    The resulting addon config is a merged of template with user defined config addon
    The values from user defined addon config take precedence over the values from template config
    and CLUSTER_NAME will be added the owner references of clusterNS/{CLUSTER_NAME}-{pacakge-short-name}-
    package

    data value secrets is created

  • customized based custom clusterboostrap and custom addonconfig:
    create clusterNS/addonconfig withname clusterNS/{Any-name-for-addon-config}
    webhook adds all undefined necessary fields.
    create clusterbootsrap clusterNS/{CLUSTER_NAME} pointing to clusterNS/{Any-name-for-addon-config}
    CLUSTER_NAME will be added the owner references of clusterNS/{Any-name-for-addon-config}
    data value secrets is created

  • customized based custom clusterboostrap and custom addonconfig with wildcard:
    create clusterNS/addonconfig withname clusterNS/{Any-name-for-addon-config}
    webhook adds all undefined necessary fields.
    create clusterbootsrap clusterNS/{CLUSTER_NAME} pointing to clusterNS/{Any-name-for-addon-config}
    CLUSTER_NAME will be added the owner references of clusterNS/{Any-name-for-addon-config}
    data value secrets is created

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

scenarios described have been added as tests using eventest library
unit tests defined

Release note

Remove requirements that user defined addon config match cluster name. 
Addon configs can be based on addonConfig templates plus user defined values by using predefined name for addon config
User defined addon configs must be used in conjunction with user defined clusterboostrap

Additional information

Special notes for your reviewer

@adduarte adduarte requested review from a team as code owners October 13, 2022 17:36
@adduarte adduarte added the do-not-merge/wip Still work in progress label Oct 13, 2022
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 2 times, most recently from a3cee36 to 9cd4948 Compare October 24, 2022 06:16
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 2 times, most recently from f5e2591 to efd4d30 Compare October 26, 2022 17:02
@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Merging #3652 (c7310e6) into main (a462bbe) will decrease coverage by 0.91%.
The diff coverage is 60.21%.

❗ Current head c7310e6 differs from pull request most recent head 196073f. Consider uploading reports for the commit 196073f to get more accurate results

@@            Coverage Diff             @@
##             main    #3652      +/-   ##
==========================================
- Coverage   46.55%   45.64%   -0.92%     
==========================================
  Files         400      426      +26     
  Lines       39797    41630    +1833     
==========================================
+ Hits        18529    19002     +473     
- Misses      19571    20898    +1327     
- Partials     1697     1730      +33     
Impacted Files Coverage Δ
addons/controllers/clusterbootstrap_controller.go 63.36% <42.85%> (+0.46%) ⬆️
...til/clusterbootstrapclone/clusterbootstrapclone.go 67.40% <54.42%> (-2.97%) ⬇️
addons/controllers/utils/utils.go 25.89% <90.62%> (ø)
cli/core/pkg/config/features.go 58.00% <0.00%> (-32.63%) ⬇️
packageclients/pkg/packageclient/package_update.go 83.57% <0.00%> (-1.43%) ⬇️
...ons/controllers/packageinstallstatus_controller.go 77.99% <0.00%> (-1.16%) ⬇️
cmd/cli/plugin/tkr/v1alpha3/os.go 73.50% <0.00%> (-0.86%) ⬇️
cli/core/pkg/pluginmanager/manager.go 62.37% <0.00%> (-0.31%) ⬇️
tkg/aws/client.go 20.33% <0.00%> (ø)
tkg/client/utils.go 36.10% <0.00%> (ø)
... and 38 more

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

@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 3 times, most recently from e155b51 to a3db048 Compare October 27, 2022 06:25
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 3 times, most recently from c7a8cc1 to 84cc150 Compare October 28, 2022 02:35
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch from 20bd5db to 9aceb35 Compare November 2, 2022 04:47
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch from 0bcd551 to a1c8c57 Compare November 2, 2022 10:13
@shyaamsn
Copy link
Contributor

shyaamsn commented Nov 2, 2022

/test install-vc7

@alfredthenarwhal
Copy link
Collaborator

Build failed! Build no: 3097

@adduarte
Copy link
Author

adduarte commented Nov 3, 2022

/test install-vc7

1 similar comment
@shyaamsn
Copy link
Contributor

shyaamsn commented Nov 3, 2022

/test install-vc7

@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch from a94dde3 to 00603ab Compare November 4, 2022 08:26
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 2 times, most recently from 83053ca to 5024131 Compare November 4, 2022 21:35
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch 2 times, most recently from b10069b to c7310e6 Compare November 5, 2022 14:38
@adduarte adduarte requested a review from shyaamsn November 5, 2022 14:44
@adduarte adduarte added ok-to-merge PRs should be labelled with this before merging and removed do-not-merge/wip Still work in progress labels Nov 5, 2022
if cbPackage != nil && cbPackage.ValuesFrom != nil && cbPackage.ValuesFrom.Inline != nil {
nonEmptyInlinePackages = append(nonEmptyInlinePackages, cbPackage)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why codecov is reporting that no test is covering this. I thought you have added a test case for this ? Could you please ensure this is covered as part of the test.

Copy link
Author

Choose a reason for hiding this comment

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

will do

Adolfo Duarte added 2 commits November 7, 2022 11:07
allowing the name of addons config to be anything

controllers changed in this patch

- Calico CNI
- Antrea CNI
- Kapp controller
- CPI
- CSI
@adduarte adduarte force-pushed the topic/adduarte/allow_addon_config_any_name branch from c7310e6 to 04c568d Compare November 7, 2022 19:07
removed unecessary if statement which was causing all nonanotated cbs to be skipped.
Copy link
Contributor

@shyaamsn shyaamsn left a comment

Choose a reason for hiding this comment

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

LGTM

@adduarte adduarte merged commit 35982bc into main Nov 8, 2022
@adduarte adduarte deleted the topic/adduarte/allow_addon_config_any_name branch November 8, 2022 01:32
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
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.

7 participants