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

Move Service interface outside of azure (cloud/ directory) package #1075

Closed
shysank opened this issue Dec 11, 2020 · 8 comments · Fixed by #1094
Closed

Move Service interface outside of azure (cloud/ directory) package #1075

shysank opened this issue Dec 11, 2020 · 8 comments · Fixed by #1094
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@shysank
Copy link
Contributor

shysank commented Dec 11, 2020

/kind cleanup

Describe the solution you'd like
We have the service interface which describes the behavior of a reconciliation capz service. There are two kinds of services:

  1. Individual service that reconciles one specific azure component (eg. virtualmachines.Service which only cares about vm reconciliation). These live inside cloud/services directory.
  2. Composite Service (aka reconcilers) that reconciles more than one individual components (eg. azuremachineReconciler which reconciles all services required for an azuremachine). These are mostly inside controllers/

Right now, the Service interface is defined inside cloud/ directory which makes it awkward for services that are outside of cloud/services to implement.

We'd like to move Service interface outside of cloud/ to a common package accessible for all implementors.

Anything else you would like to add:

More discussion on this topic here

Environment:

  • cluster-api-provider-azure version:
  • Kubernetes version: (use kubectl version):
  • OS (e.g. from /etc/os-release):
@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Dec 11, 2020
@surajrana93
Copy link
Contributor

@shysank
Can i take this up if its unassigned?

@shysank
Copy link
Contributor Author

shysank commented Dec 15, 2020

Sure @surajrana93 , although I'm not entirely sure about where we want to move Service to? Should we create a new package?

/cc @CecileRobertMichon @devigned

@devigned
Copy link
Contributor

Maybe we just rename azure.Service to azure.Reconciler. Thoughts?

Related to this, does anyone else find that the cloud directory contains the azure package a bit odd? Should it be the azure directory or the cloud package? I prefer the former.

@CecileRobertMichon
Copy link
Contributor

Maybe we just rename azure.Service to azure.Reconciler. Thoughts?

+1 for the rename

Related to this, does anyone else find that the cloud directory contains the azure package a bit odd? Should it be the azure directory or the cloud package? I prefer the former.

Yep, I think this slipped into the v1alpha2 revamp and never got corrected. +1 for renaming the directory to azure

@shysank
Copy link
Contributor Author

shysank commented Dec 15, 2020

+1 for renaming both interface, and directory. I also noticed we have an OldService interface which is only used in machine pool. Not sure about the history here, but should we also migrate it to implement the new service interface?

@CecileRobertMichon
Copy link
Contributor

I also noticed we have an OldService interface which is only used in machine pool. Not sure about the history here, but should we also migrate it to implement the new service interface?

Yep that's leftover TODO from #757

As soon as all the services have been refactored we can get rid of OldService

@surajrana93
Copy link
Contributor

/assign

@surajrana93
Copy link
Contributor

@CecileRobertMichon
Please take a look when you get time #1094

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
5 participants