Skip to content
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

[GoodFirst] Readability enhancement #1235

Closed
RainbowMango opened this issue Jan 11, 2022 · 5 comments · Fixed by #1242
Closed

[GoodFirst] Readability enhancement #1235

RainbowMango opened this issue Jan 11, 2022 · 5 comments · Fixed by #1242
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.

Comments

@RainbowMango
Copy link
Member

Task description:
When I went through the code again, found some pieces of code that could be improved for better readability.

This task only focus on syncWorkStatus and reflectStatus function:

  • modify some outdated comments
  • rename variable names(obj --> observedObj, desireObj-->desiredObj)

Solution:

diff --git a/pkg/controllers/status/workstatus_controller.go b/pkg/controllers/status/workstatus_controller.go
index 1b0ce372..8487fe72 100644
--- a/pkg/controllers/status/workstatus_controller.go
+++ b/pkg/controllers/status/workstatus_controller.go
@@ -159,7 +159,7 @@ func (c *WorkStatusController) syncWorkStatus(key util.QueueKey) error {
                return fmt.Errorf("invalid key")
        }

-       obj, err := helper.GetObjectFromCache(c.RESTMapper, c.InformerManager, fedKey, c.Client, c.ClusterClientSetFunc)
+       observedObj, err := helper.GetObjectFromCache(c.RESTMapper, c.InformerManager, fedKey, c.Client, c.ClusterClientSetFunc)
        if err != nil {
                if apierrors.IsNotFound(err) {
                        return c.handleDeleteEvent(fedKey)
@@ -167,8 +167,8 @@ func (c *WorkStatusController) syncWorkStatus(key util.QueueKey) error {
                return err
        }

-       workNamespace := util.GetLabelValue(obj.GetLabels(), workv1alpha1.WorkNamespaceLabel)
-       workName := util.GetLabelValue(obj.GetLabels(), workv1alpha1.WorkNameLabel)
+       workNamespace := util.GetLabelValue(observedObj.GetLabels(), workv1alpha1.WorkNamespaceLabel)
+       workName := util.GetLabelValue(observedObj.GetLabels(), workv1alpha1.WorkNameLabel)
        if len(workNamespace) == 0 || len(workName) == 0 {
                klog.Infof("Ignore object(%s) which not managed by karmada.", fedKey.String())
                return nil
@@ -185,8 +185,7 @@ func (c *WorkStatusController) syncWorkStatus(key util.QueueKey) error {
                return err
        }

-       // consult with version manager if current status needs update.
-       desireObj, err := c.getRawManifest(workObject.Spec.Workload.Manifests, obj)
+       desiredObj, err := c.getRawManifest(workObject.Spec.Workload.Manifests, observedObj)
        if err != nil {
                return err
        }
@@ -197,14 +196,15 @@ func (c *WorkStatusController) syncWorkStatus(key util.QueueKey) error {
                return err
        }

-       // compare version to determine if need to update resource
-       needUpdate, err := c.ObjectWatcher.NeedsUpdate(clusterName, desireObj, obj)
+       // we should check if the observed status is consistent with the declaration to prevent accidental changes made
+       // in member clusters.
+       needUpdate, err := c.ObjectWatcher.NeedsUpdate(clusterName, desiredObj, observedObj)
        if err != nil {
                return err
        }

        if needUpdate {
-               if err := c.ObjectWatcher.Update(clusterName, desireObj, obj); err != nil {
+               if err := c.ObjectWatcher.Update(clusterName, desiredObj, observedObj); err != nil {
                        klog.Errorf("Update %s failed: %v", fedKey.String(), err)
                        return err
                }
@@ -217,8 +217,8 @@ func (c *WorkStatusController) syncWorkStatus(key util.QueueKey) error {
                // status changes.
        }

-       klog.Infof("reflecting %s(%s/%s) status to Work(%s/%s)", obj.GetKind(), obj.GetNamespace(), obj.GetName(), workNamespace, workName)
-       return c.reflectStatus(workObject, obj)
+       klog.Infof("reflecting %s(%s/%s) status to Work(%s/%s)", observedObj.GetKind(), observedObj.GetNamespace(), observedObj.GetName(), workNamespace, workName)
+       return c.reflectStatus(workObject, observedObj)
 }

 func (c *WorkStatusController) handleDeleteEvent(key keys.FederatedKey) error {
@@ -267,7 +267,7 @@ func (c *WorkStatusController) recreateResourceIfNeeded(work *workv1alpha1.Work,
        return nil
 }

-// reflectStatus grabs cluster object's running status then updates to it's owner object(Work).
+// reflectStatus grabs cluster object's running status then updates to its owner object(Work).
 func (c *WorkStatusController) reflectStatus(work *workv1alpha1.Work, clusterObj *unstructured.Unstructured) error {
        statusMap, _, err := unstructured.NestedMap(clusterObj.Object, "status")
        if err != nil {

Who can join or take the task:

The good first issue is intended for first-time contributors to get started on his/her contributor journey.

After a contributor has successfully completed 1-2 good first issue's,
they should be ready to move on to help wanted items, saving the remaining good first issue for other new contributors.

How to join or take the task:

Just reply on the issue with the message /assign in a separate line.

Then, the issue will be assigned to you.

How to ask for help:

If you need help or have questions, please feel free to ask on this issue.
The issue author or other members of the community will guide you through the contribution process.

@RainbowMango RainbowMango added the good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. label Jan 11, 2022
@Fish-pro
Copy link
Member

/assign

@RainbowMango
Copy link
Member Author

@Fish-pro It's yours now, appreciate it!

@vipulsaxena31
Copy link

/assign

@RainbowMango
Copy link
Member Author

@vipulsaxena31 Thanks but this issue has been closed(solved).

@janup2442
Copy link

Hello sir , As this issue was closed now . I am new in open source contribution , and I am looking for basic good First issues , so that I can understand and expand my experience in this field.

Do /assign me some basic or Good first issues whenever available . Thank you !!

@Fish-pro Fish-pro removed their assignment Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants