✨ addon: add support for multiple GVK #585
Conversation
adds test cases updates mergeAddonConfig function to match new multi same-gvk configs Signed-off-by: Joao Marcal <jmarcal@redhat.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #585 +/- ##
==========================================
+ Coverage 62.60% 63.74% +1.14%
==========================================
Files 181 181
Lines 13964 14039 +75
==========================================
+ Hits 8742 8949 +207
+ Misses 4338 4195 -143
- Partials 884 895 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
haoqing0110
force-pushed
the
multiple-gvk
branch
3 times, most recently
from
19d6f36 to
74d73d7
Compare
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
multiple-gvk
branch
from
74d73d7 to
4c7ce51
Compare
|
/assign @qiujian16 |
|
@haoqing0110: GitHub didn't allow me to request PR reviews from the following users: JoaoBraveCoding. Note that only open-cluster-management-io members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
| for _, config := range mcaCopy.Status.ConfigReferences { | ||
| if _, ok := desiredConfigMap[config.ConfigGroupResource]; ok { | ||
| mergedConfigs = append(mergedConfigs, config) | ||
| for _, configRef := range mcaCopy.Status.ConfigReferences { |
There was a problem hiding this comment.
add some comments for each for loop
| output := addonConfigMap{} | ||
| for k, v := range d { | ||
| output[k] = v | ||
| func (m addonConfigMap) copy() addonConfigMap { |
There was a problem hiding this comment.
yes, update the code.
| @@ -461,3 +495,53 @@ func rolloutStatusHasCluster(clusterRolloutStatus []clustersdkv1alpha1.ClusterRo | |||
| } | |||
| return false | |||
| } | |||
|
|
|||
| func overrideConfigMapByInstallConfigRef( | |||
| } | ||
| } | ||
|
|
||
| func overrideConfigMapByAddOnConfigs( |
There was a problem hiding this comment.
ditto we need more docs for matinability
| addOnConfigs []addonv1alpha1.AddOnConfig, | ||
| desiredConfigs map[addonv1alpha1.ConfigGroupResource][]addonv1alpha1.ConfigReferent, | ||
| ) { | ||
| gvkOverwritten := make(map[addonv1alpha1.ConfigGroupResource]struct{}) |
There was a problem hiding this comment.
yes, have modified the code to use sets.
| @@ -48,14 +48,14 @@ type addonNode struct { | |||
| status *clustersdkv1alpha1.ClusterRolloutStatus | |||
| } | |||
|
|
|||
| type addonConfigMap map[addonv1alpha1.ConfigGroupResource]addonv1alpha1.ConfigReference | |||
| type addonConfigMap map[addonv1alpha1.ConfigGroupResource][]addonv1alpha1.ConfigReference | |||
There was a problem hiding this comment.
would it be easier if we use something like
type addonConfigMap map[addonv1alpha1.ConfigGroupResource]set.Set[addonv1alpha1.ConfigReference]?
There was a problem hiding this comment.
For the ConfigReference, we use it to store ConfigGroupResource, DesiredConfig, LastObservedGeneration and LastAppliedConfig, while only use ConfigGroupResource and DesiredConfig.ConfigReferent when doing comparison, it seems not be able to use the strengths of sets, like Has().
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
multiple-gvk
branch
from
1c0879c to
ca344f4
Compare
|
@JoaoBraveCoding could you help take a look? |
|
@iblancasa: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, iblancasa, qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
haoqing0110
changed the title
|
/lgtm |
openshift-merge-bot
bot
merged commit dfa835d
into
open-cluster-management-io:main
Summary
Related issue(s)
Ref: #547