-
Notifications
You must be signed in to change notification settings - Fork 919
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
retain for hpa controlled Deployment resource (labels method) #4078
Conversation
de36516
to
0aa8191
Compare
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #4078 +/- ##
==========================================
- Coverage 53.79% 53.51% -0.29%
==========================================
Files 232 234 +2
Lines 23144 23281 +137
==========================================
+ Hits 12450 12458 +8
- Misses 10016 10142 +126
- Partials 678 681 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
@chaunceyjiang hello, for yesterday's hpa retain problem, I also achieve |
@XiShanYongYe-Chang it is ready to review |
ee4abc1
to
28f481f
Compare
I have refactored my code to adapt to #4072, avoiding potential conflict. @XiShanYongYe-Chang |
Hi @chaosi-zju, #4072 has been merged, can you help rebase it? |
/assign |
28f481f
to
8d20d86
Compare
done |
Hi @chaosi-zju I'm sorry for noticing this late. Thanks for your refactoring, I will help review it now. |
8d20d86
to
2bc7dff
Compare
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_predicate.go
Outdated
Show resolved
Hide resolved
088643c
to
53a3032
Compare
53a3032
to
61644dd
Compare
61644dd
to
64ac77c
Compare
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.
Thanks a lot~
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_controller.go
Outdated
Show resolved
Hide resolved
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_predicate.go
Outdated
Show resolved
Hide resolved
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_predicate.go
Outdated
Show resolved
Hide resolved
pkg/controllers/hpareplicassyncer/hpa_replicas_syncer_predicate.go
Outdated
Show resolved
Hide resolved
abf54a8
to
7a320a2
Compare
Signed-off-by: chaosi-zju <[email protected]>
7a320a2
to
a4828cc
Compare
Ask an again review from @chaunceyjiang @lxtywypc |
Thanks~ |
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.
/approve
@chaunceyjiang @lxtywypc
Please feel free to leave your comments here or #4058 if we missed something :) We still have time to iterate it.
[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 |
What type of PR is this?
/kind feature
What this PR does / why we need it:
Write a controller to watch the creation of HPA. Once there is a change in HPA, It will add a label to the target resource pointed by HPA. Then we only need to check if the resource has this specific label here to determine whether we need to perform the retain operation.
Which issue(s) this PR fixes:
part of #4058
Special notes for your reviewer:
another mthod:#4075we should choose just one between two.
Test Method:same as #4064
Test Report:
Does this PR introduce a user-facing change?: