-
Notifications
You must be signed in to change notification settings - Fork 94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ sync between ManagedCluster and cluster inventory API #615
✨ sync between ManagedCluster and cluster inventory API #615
Conversation
/hold more UT to add |
/assign @qiujian16 |
|
||
// sync status.properties | ||
cpProperties := []cpv1alpha1.Property{} | ||
for k, v := range managedCluster.GetLabels() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not get labels, but clusterclaims in the status
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
if modified { | ||
_, err := c.patcher.PatchLabelAnnotations(ctx, newClusterProfile, newClusterProfile.ObjectMeta, clusterProfile.ObjectMeta) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directly return here, we can update status in the next loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
e1ff679
to
4633a1e
Compare
4633a1e
to
21c4f1d
Compare
21c4f1d
to
77dd048
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #615 +/- ##
==========================================
+ Coverage 64.07% 64.08% +0.01%
==========================================
Files 181 182 +1
Lines 14166 14277 +111
==========================================
+ Hits 9077 9150 +73
- Misses 4186 4214 +28
- Partials 903 913 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -49,6 +49,7 @@ var ( | |||
"cluster-manager/hub/0000_03_addon.open-cluster-management.io_addontemplates.crd.yaml", | |||
"cluster-manager/hub/0000_03_clusters.open-cluster-management.io_placementdecisions.crd.yaml", | |||
"cluster-manager/hub/0000_05_clusters.open-cluster-management.io_addonplacementscores.crd.yaml", | |||
"cluster-manager/hub/0000_00_multicluster.x-k8s.io_clusterprofiles.crd.yaml", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not install this crd when feature is diabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concern is whether should we remove the crd (together with all the crs) when disabled, any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not need to remove when disable, but since the feature is disabled by default, I do not want the crd is surprisingly created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
77dd048
to
db2a5f8
Compare
db2a5f8
to
2a5414a
Compare
// CRD resource files to clean | ||
hubDeployCRDResources := hubCRDResourceFiles | ||
|
||
// Include the ClusterProfile CRD in case it was installed, since the CRD will not be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the comment said the crd will not be removed, so why should we clean up it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ClusterProfile crd is not removed when featuregates disable, but will be cleaned when ClusterManager is deleting, the behavior of disable and clean (except install) are the same as other crds.
But this is a good question since ClusterProfile CRD might be used by other project, might need to leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes let's just leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
Signed-off-by: haoqing0110 <qhao@redhat.com>
2a5414a
to
0b204cb
Compare
/approve |
[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 |
/unhold |
d9ab252
into
open-cluster-management-io:main
Summary
Related issue(s)
#247