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.

Sign up for GitHub

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

Jump to bottom

Vpa - Component status #2742

Merged
merged 1 commit into from
Jan 22, 2020
Merged

Vpa - Component status #2742

merged 1 commit into from
Jan 22, 2020

Conversation

krzysied
Copy link
Contributor

Adding component status implementation based on Leases.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2020
@krzysied
Copy link
Contributor Author

/assign @bskiba

@krzysied krzysied mentioned this pull request Jan 16, 2020
Closed
@krzysied
Copy link
Contributor Author

Vendor update moved to #2743
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2020
@krzysied krzysied force-pushed the vpa_admission_controller_status branch from 8b0322a to 6d94edf Compare January 16, 2020 13:40
bskiba
bskiba reviewed Jan 16, 2020
View reviewed changes
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

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

I don't wan't to encourage large pull requests, but I feel like I'm kinda missing context reviewing an unused class.
Please rebase to master once #2743 merges

vertical-pod-autoscaler/pkg/utils/status/status_object.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object.go Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object.go Outdated Show resolved Hide resolved
@krzysied krzysied force-pushed the vpa_admission_controller_status branch from 6d94edf to c74013c Compare January 17, 2020 09:53
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 17, 2020
@krzysied krzysied force-pushed the vpa_admission_controller_status branch 2 times, most recently from e1132a0 to ae0e75b Compare January 17, 2020 11:42
bskiba
bskiba reviewed Jan 17, 2020
View reviewed changes
Copy link
Member

@bskiba bskiba left a comment

Choose a reason for hiding this comment

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

Couple more comments.

Thanks for adding comprehensive set of tests for this 👍

vertical-pod-autoscaler/pkg/utils/status/status_object.go Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object.go Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object_test.go
return true, nil, fmt.Errorf("unsupported action")
}
}(),
wantErr: false,
Copy link
Member

Choose a reason for hiding this comment

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

No case with wantErr = true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test case added.

vertical-pod-autoscaler/pkg/utils/status/status_object_test.go Outdated Show resolved Hide resolved
vertical-pod-autoscaler/pkg/utils/status/status_object_test.go Outdated
}
for _, tc := range tests {
t.Run(fmt.Sprintf("test case: %s", tc.name), func(t *testing.T) {
assert.Equal(t, IsStatusValid(tc.lease, tc.leaseTimeout), tc.want)
Copy link
Member

Choose a reason for hiding this comment

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

Please add a message for failed equality check, otherwise we get "expected: false got: true" only on test failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated test names to be more self-explanatory. Imho simple "expected, got" is enough.

vertical-pod-autoscaler/pkg/utils/status/status_object_test.go Outdated Show resolved Hide resolved
@krzysied krzysied force-pushed the vpa_admission_controller_status branch from ae0e75b to 208d937 Compare January 20, 2020 12:15
@krzysied
Copy link
Contributor Author

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
@bskiba
Copy link
Member

bskiba commented Jan 22, 2020

One more comment to the tests

@krzysied krzysied force-pushed the vpa_admission_controller_status branch 2 times, most recently from 8ae0ccd to 26c3320 Compare January 22, 2020 12:45
@krzysied krzysied force-pushed the vpa_admission_controller_status branch from 26c3320 to 6b5e8bd Compare January 22, 2020 12:48
bskiba
bskiba approved these changes Jan 22, 2020
View reviewed changes
Copy link
Member

@bskiba bskiba 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
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bskiba

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5cbd278 into kubernetes:master Jan 22, 2020
@krzysied krzysied deleted the vpa_admission_controller_status branch January 22, 2020 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bskiba bskiba bskiba approved these changes

@jbartosik jbartosik Awaiting requested review from jbartosik

@mwielgus mwielgus Awaiting requested review from mwielgus

Assignees

@bskiba bskiba

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@krzysied @bskiba @k8s-ci-robot