✨ set configured condition in mca #635
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #635 +/- ##
==========================================
+ Coverage 63.90% 64.08% +0.17%
==========================================
Files 180 181 +1
Lines 14116 14166 +50
==========================================
+ Hits 9021 9078 +57
+ Misses 4192 4185 -7
Partials 903 903
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
/assign @qiujian16 @elgnay |
| if configured.Has(addon.mca.Namespace) { | ||
| continue | ||
| } | ||
| if !meta.IsStatusConditionFalse(addon.mca.Status.Conditions, addonv1alpha1.ManagedClusterAddOnConditionConfigured) { |
| // should set condition to true. | ||
| // 2. Addons with configurations and already rollout successfully. In upgrade scenario, when the | ||
| // addon configurations do not change while addon components upgrade, should set condition to true. | ||
| for _, addon := range graph.getAddonsSucceeded() { |
There was a problem hiding this comment.
this is not clear to me, should toUpdate and toApply include all addons need to be set? ToSuccess always comes from toUpdate.
There was a problem hiding this comment.
For addons that don't define any configs, the rollout status will be set to success insetRolloutStatus directly, and will never appear in the toUpdate or toApply.
Another case is for example, addon defines config A, and in next release, both addon-framework and addon-manager upgrades while it keeps using config A, since the desired config already matches the actual, will not have change to add configured condition in toUpdate or toApply.
| @@ -96,6 +97,11 @@ func (a *CRDTemplateAgentAddon) Manifests( | |||
| cluster *clusterv1.ManagedCluster, | |||
| addon *addonapiv1alpha1.ManagedClusterAddOn) ([]runtime.Object, error) { | |||
|
|
|||
| if !meta.IsStatusConditionTrue(addon.Status.Conditions, addonapiv1alpha1.ManagedClusterAddOnConditionConfigured) { | |||
There was a problem hiding this comment.
if we have addon-framework check the conds, do we still need this?
There was a problem hiding this comment.
Currently, the code to "check the conds" is added at the beginning of each implementation of Manifests(), so both template and addon-framework have this piece of code.
There was a problem hiding this comment.
will try to find a better place to add the check.
| Status: metav1.ConditionTrue, | ||
| Reason: "ConfigurationsConfigured", | ||
| Message: "Configurations configured", | ||
| }) |
There was a problem hiding this comment.
we need an integration test for the case that only when condition is set, the addon-template will start render.
haoqing0110
force-pushed
the
br_config-condition
branch
2 times, most recently
from
2ff21d5 to
27ae996
Compare
Signed-off-by: haoqing0110 <qhao@redhat.com>
haoqing0110
force-pushed
the
br_config-condition
branch
2 times, most recently
from
2a2aca7 to
bdd22c7
Compare
|
Have updated the code based on comments and will create another PR to update the framework and add testing case for addon template. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: haoqing0110, 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 |
openshift-merge-bot
bot
merged commit 851d015
into
open-cluster-management-io:main
Summary
Related issue(s)
Fixes #