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

Fix: remove the setTransitionTime from cluster status controller. #3316

Merged
merged 1 commit into from
Mar 23, 2023
Merged

Fix: remove the setTransitionTime from cluster status controller. #3316

merged 1 commit into from
Mar 23, 2023

Conversation

xigang
Copy link
Member

@xigang xigang commented Mar 22, 2023

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

There is no need to call setTransitionTime before calling the SetStatusCondition method. From the code logic, call setTransitionTime is not required.

https://github.com/karmada-io/karmada/blob/master/pkg/controllers/status/cluster_status_controller.go#L180 and https://github.com/karmada-io/karmada/blob/master/pkg/controllers/status/cluster_status_controller.go#L205

Which issue(s) this PR fixes:
Fixes ##3313

Special notes for your reviewer:

@XiShanYongYe-Chang

Does this PR introduce a user-facing change?:

NONE

@karmada-bot karmada-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 22, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #3316 (1dd58a5) into master (a14d64f) will decrease coverage by 0.01%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #3316      +/-   ##
==========================================
- Coverage   49.13%   49.12%   -0.01%     
==========================================
  Files         209      209              
  Lines       18754    18751       -3     
==========================================
- Hits         9214     9212       -2     
+ Misses       9039     9038       -1     
  Partials      501      501              
Flag Coverage Δ
unittests 49.12% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kg/controllers/status/cluster_status_controller.go 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

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

Copy link
Member

@Poor12 Poor12 left a comment

Choose a reason for hiding this comment

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

Could you delete setTransitionTime together? Seems this function is non-used.

@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 22, 2023
@xigang
Copy link
Member Author

xigang commented Mar 22, 2023

Could you delete setTransitionTime together? Seems this function is non-used.

setTransitionTime has been deleted

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

Can you have a test on it to see if the condition transition time is set as expected?

It should be ok, but one more test would be better.

Copy link
Member

@XiShanYongYe-Chang XiShanYongYe-Chang left a comment

Choose a reason for hiding this comment

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

Thanks~
/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2023
@xigang
Copy link
Member Author

xigang commented Mar 22, 2023

@XiShanYongYe-Chang @RainbowMango need to approve:)

@XiShanYongYe-Chang
Copy link
Member

XiShanYongYe-Chang commented Mar 22, 2023

Have you made a test on your local site?

@xigang
Copy link
Member Author

xigang commented Mar 22, 2023

Have you made a test on your local site?

I'll add some unit test cases a little later.

@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 22, 2023
@xigang
Copy link
Member Author

xigang commented Mar 22, 2023

Can you have a test on it to see if the condition transition time is set as expected?

It should be ok, but one more test would be better.

@RainbowMango @XiShanYongYe-Chang unit tests have been added, take a look.

@XiShanYongYe-Chang
Copy link
Member

Hi, @xigang thanks for your work. I think we don't need to add ut to the method in the third-party library, but judging from your new ut context, what @RainbowMango says is satisfied.

Can you help remove the newly added ut, and we can continue to approve?

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

Sorry, I didn't make it clear.

Can you have a test on it to see if the condition transition time is set as expected?

I mean we can test it manually would be fine. No worries, I can help do it.
But, please remove the unit test.

pkg/util/condition_test.go Outdated Show resolved Hide resolved
…roller

Signed-off-by: xigang <wangxigang2014@gmail.com>
@karmada-bot karmada-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 23, 2023
@xigang
Copy link
Member Author

xigang commented Mar 23, 2023

@XiShanYongYe-Chang @RainbowMango The newly added unit tests have been removed. I only have the local environment, can you help verify it in your test environment?

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

It works. Thanks.
/lgtm
/approve

Ready condition:

  - lastTransitionTime: "2023-03-23T02:01:33Z"
    message: cluster is healthy and ready to accept workloads
    reason: ClusterReady
    status: "True"
    type: Ready

Not Ready condition:

  - lastTransitionTime: "2023-03-23T02:06:53Z" ## We can see the transitionTime is updated as expected.
    message: cluster is not reachable
    reason: ClusterNotReachable
    status: "False"
    type: Ready

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2023
@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2023
@karmada-bot karmada-bot merged commit d349f00 into karmada-io:master Mar 23, 2023
@xigang
Copy link
Member Author

xigang commented Mar 23, 2023

It works. Thanks. /lgtm /approve

Ready condition:

  - lastTransitionTime: "2023-03-23T02:01:33Z"
    message: cluster is healthy and ready to accept workloads
    reason: ClusterReady
    status: "True"
    type: Ready

Not Ready condition:

  - lastTransitionTime: "2023-03-23T02:06:53Z" ## We can see the transitionTime is updated as expected.
    message: cluster is not reachable
    reason: ClusterNotReachable
    status: "False"
    type: Ready

@RainbowMango Thanks for help verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants