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

Add Network Describer interface #984

Merged
merged 1 commit into from
Oct 9, 2020
Merged

Add Network Describer interface #984

merged 1 commit into from
Oct 9, 2020

Conversation

CecileRobertMichon
Copy link
Contributor

What type of PR is this?
/kind cleanup

What this PR does / why we need it: Not every service need to know about Networking. This makes it more granular so that services such as groups or roleassignments can rely only on basic cluster info. It's a stepping stone to removing AzureCluster from MachineScope, so MachineScope can use different Cluster/ControlPlane providers (ie. AzureManagedControlPlane).

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #824

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests

Release note:

Add Network Describer interface

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/provider/azure Issues or PRs related to azure provider labels Oct 7, 2020
@k8s-ci-robot k8s-ci-robot added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label Oct 7, 2020
@CecileRobertMichon CecileRobertMichon changed the title 💎 Add Network Describer interface Add Network Describer interface Oct 7, 2020
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 7, 2020
@CecileRobertMichon CecileRobertMichon force-pushed the network-describer branch 2 times, most recently from 5605d06 to e76d20d Compare October 7, 2020 22:34
@CecileRobertMichon
Copy link
Contributor Author

/assign @alexeldeib

devigned
devigned reviewed Oct 8, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

Just a couple comments. Overall looks good. Glad to see the decomposition and more specific interfaces.

cloud/interfaces.go Outdated
AdditionalTags() infrav1.Tags
}

type ClusterScope interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels a little odd for ClusterScope to be an interface when all the other *Scope types are structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed... naming is hard

I need one thing that describes the whole k8s Cluster with networking, and one that describes the basic cluster info like resource group and cluster name. Any suggestions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ClusterScoper? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

Copy link
Contributor

Choose a reason for hiding this comment

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

ClusterPlan?

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 think I like ClusterScoper better than ClusterPlan because it describes what it is more closely and follows the interface naming convention by ending with er

cloud/services/disks/service.go Outdated Show resolved Hide resolved
devigned
devigned approved these changes Oct 9, 2020
View reviewed changes
Copy link
Contributor

@devigned devigned left a comment

Choose a reason for hiding this comment

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

lgtm

@nader-ziada
Copy link
Contributor

/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 Oct 9, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nader-ziada

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 Oct 9, 2020
@nader-ziada
Copy link
Contributor

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 9, 2020 •
edited
Loading

@CecileRobertMichon: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-provider-azure-apidiff f621f73 link /test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit a985e1e into kubernetes-sigs:master Oct 9, 2020
@CecileRobertMichon CecileRobertMichon deleted the network-describer branch March 19, 2021 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nader-ziada nader-ziada nader-ziada left review comments

@devigned devigned devigned approved these changes

@cpanato cpanato Awaiting requested review from cpanato

Assignees

@alexeldeib alexeldeib

@nader-ziada nader-ziada

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/azure Issues or PRs related to azure provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add network describer interface
5 participants
@CecileRobertMichon @nader-ziada @k8s-ci-robot @devigned @alexeldeib