-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(CollaSet): supports operationDelaySeconds in Scaling-in and Updating Pods #79
Conversation
8f33df2
to
e96020f
Compare
e96020f
to
bfe0cf8
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.
ptal
// update status anyway | ||
if err := r.updateStatus(ctx, instance, newStatus); err != nil { | ||
return ctrl.Result{}, fmt.Errorf("fail to update status of CollaSet %s: %s", req, err) | ||
return ctrl.Result{RequeueAfter: requeueAfter}, fmt.Errorf("fail to update status of CollaSet %s: %s", req, 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.
%v should be used for req and 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.
no need to fix : )
podWrappers, newStatus, syncErr := r.doSync(instance, updatedRevision, revisions, newStatus) | ||
return calculateStatus(instance, newStatus, updatedRevision, podWrappers, syncErr), syncErr | ||
func (r *CollaSetReconciler) DoReconcile(instance *appsv1alpha1.CollaSet, updatedRevision *appsv1.ControllerRevision, revisions []*appsv1.ControllerRevision, newStatus *appsv1alpha1.CollaSetStatus) (time.Duration, *appsv1alpha1.CollaSetStatus, error) { | ||
podWrappers, newStatus, requeueAfter, syncErr := r.doSync(instance, updatedRevision, revisions, newStatus) |
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.
DoReconcile just call doSync, and doSync only called by DoReconcile, is it possible to merged DoReconcile and doSync to one func
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.
no need to fix : )
…eration
1. Does this PR affect any open issues?(Y/N) and add issue references (e.g. "fix #123", "re #123".):
re FeatureRequest: CollaSet supports delaying a period before do scaling-in or updating Pods #78
2. What is the scope of this PR (e.g. component or file name):
3. Provide a description of the PR(e.g. more details, effects, motivations or doc link):
4. Are there any breaking changes?(Y/N) and describe the breaking changes(e.g. more details, motivations or doc link):
5. Are there test cases for these changes?(Y/N) select and add more details, references or doc links:
6. Release note
Please refer to Release Notes Language Style Guide to write a quality release note.