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

Create Health Checks Provider to interact with Google Cloud #1763

Merged
merged 1 commit into from
Sep 9, 2022
Merged

Create Health Checks Provider to interact with Google Cloud #1763

merged 1 commit into from
Sep 9, 2022

Conversation

panslava
Copy link
Contributor

@panslava panslava commented Jul 29, 2022 •
edited
Loading

This separates business logic of managing L4 HealthChecks and interacting with Google Cloud API

We want to achieve following layered level logic
l4controller (handles workqueue for l4 services) -> l4 (for single service) -> l4healthchecks (health checks business logic) -> healthchecksprovider (no business logic, only interaction with gcp)

This makes composition better, testing and mocking should be also easier in the long run

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2022
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 29, 2022
@panslava panslava force-pushed the create-hc-provider branch 3 times, most recently from 1834d41 to 270f6a8 Compare July 29, 2022 11:56
@panslava
Copy link
Contributor Author

panslava commented Jul 29, 2022 •
edited
Loading

/assign cezarygerard

1 similar comment
@panslava
Copy link
Contributor Author

/assign cezarygerard

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2022
@panslava panslava force-pushed the create-hc-provider branch from 270f6a8 to 1e860f6 Compare August 1, 2022 15:51
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2022
@panslava panslava force-pushed the create-hc-provider branch from 1e860f6 to a535c7b Compare August 1, 2022 15:52
@panslava
Copy link
Contributor Author

panslava commented Aug 1, 2022

I personally a bit unsure about folder structure, maybe reviewers will have some opinion

@panslava
Copy link
Contributor Author

panslava commented Aug 2, 2022

/assign bowei

@panslava panslava force-pushed the create-hc-provider branch 3 times, most recently from 3b9a44f to b8c794c Compare August 18, 2022 12:36
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 18, 2022
@panslava panslava force-pushed the create-hc-provider branch from b8c794c to 667055e Compare August 18, 2022 13:02
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2022
@panslava panslava force-pushed the create-hc-provider branch from 667055e to 9eb63b1 Compare August 26, 2022 08:30
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 26, 2022
@panslava panslava force-pushed the create-hc-provider branch from 9eb63b1 to 907cf9c Compare August 31, 2022 13:26
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2022
@panslava panslava force-pushed the create-hc-provider branch 7 times, most recently from 0806833 to cb02f09 Compare September 5, 2022 08:57
@cezarygerard
Copy link
Contributor

Slavik, please add some more to the description

what is our idea for splitting the logic into 4 layers

  • controller
  • work item
  • provider
  • api wrapper

cezarygerard
cezarygerard reviewed Sep 5, 2022
View reviewed changes
pkg/healthchecksprovider/healthchecksprovider.go Outdated Show resolved Hide resolved
pkg/healthchecksprovider/healthchecksprovider_test.go Outdated Show resolved Hide resolved
pkg/healthchecksprovider/healthchecksprovider_test.go Show resolved Hide resolved
pkg/healthchecksl4/healthchecksl4.go
@@ -14,14 +14,13 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package healthchecks
package healthchecksl4
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe it's better to name the package l4healthchecks?

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT?

pkg/healthchecksl4/interfaces.go Outdated Show resolved Hide resolved
@panslava
Copy link
Contributor Author

panslava commented Sep 5, 2022

/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 Sep 5, 2022
@panslava
Copy link
Contributor Author

panslava commented Sep 5, 2022 •
edited
Loading

will squash after lgtm

@panslava panslava mentioned this pull request Sep 5, 2022
Merged
@cezarygerard
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 7, 2022
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2022
@panslava
Create Health Checks Provider to interact with Google Cloud
5767597 
This separates business logic and handling health checks resources
@cezarygerard
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cezarygerard, panslava

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:
  • OWNERS [cezarygerard,panslava]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@panslava
Copy link
Contributor Author

panslava commented Sep 9, 2022

/unhold

@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 Sep 9, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4312cb9 into kubernetes:master Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cezarygerard cezarygerard cezarygerard left review comments

@swetharepakula swetharepakula Awaiting requested review from swetharepakula

@thockin thockin Awaiting requested review from thockin

Assignees

@bowei bowei

@cezarygerard cezarygerard

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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@panslava @cezarygerard @k8s-ci-robot @bowei