-
Notifications
You must be signed in to change notification settings - Fork 431
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
🏃 Remove all TODOs in the codebase #555
🏃 Remove all TODOs in the codebase #555
Conversation
@@ -19,11 +19,7 @@ hybrid deployments of Kubernetes. | |||
|
|||
Check out the [Cluster API Quick Start][quickstart] to create your first Kubernetes cluster on Azure using Cluster API. | |||
|
|||
## Features | |||
|
|||
TODO |
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.
// TODO: Investigate if we should reference resources in other ways | ||
} | ||
|
||
// TODO: Investigate resource filters |
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.
resolved: Azure doesn't have resource filters, it's an AWS concept only (Azure has resource groups).
// TODO: ResourceSpec should be removed once concrete specs have been defined for all Azure resources in use. | ||
// type ResourceSpec interface{} | ||
|
||
// TODO: Write type tests |
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.
resolved: there are no testable functions in this file, only type definitions
@@ -23,22 +23,6 @@ import ( | |||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |||
) | |||
|
|||
// // ResourceSpec defines a generic spec that can used to define Azure resources. | |||
// TODO: ResourceSpec should be removed once concrete specs have been defined for all Azure resources in use. | |||
// type ResourceSpec interface{} |
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.
resolved: removed this as it is unused and we have individual specs for the Azure resources used.
// ID of resource | ||
// +optional | ||
ID *string `json:"id,omitempty"` | ||
// TODO: Investigate if we should reference resources in other ways |
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.
resolved: removed AzureResourceReference
as it is unused in the codebase and Azure does not have several ways for referencing resources.
@@ -157,14 +141,6 @@ type SecurityGroup struct { | |||
Tags Tags `json:"tags,omitempty"` | |||
} | |||
|
|||
/* | |||
// TODO |
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.
resolved: dead code
@@ -197,45 +173,10 @@ type IngressRule struct { | |||
Destination *string `json:"destination,omitempty"` | |||
} | |||
|
|||
// TODO |
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.
resolved: dead code
// IngressRules is a slice of Azure ingress rules for security groups. | ||
type IngressRules []*IngressRule | ||
|
||
// TODO |
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.
resolved: dead code
// PublicIP defines an Azure public IP address. | ||
// TODO: Remove once load balancer is implemented. |
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.
resolved: load balancer is already implemented but we still reference this type in the Network
struct for APIServerIP
.
@@ -75,7 +75,6 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error { | |||
return errors.New("Invalid Subnet Specification") | |||
} | |||
if subnet, err := s.Get(ctx, subnetSpec); err == nil { | |||
// TODO: add validation on existing subnet |
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.
@@ -78,12 +78,6 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error { | |||
return errors.Wrapf(err, "cannot create vm extension") | |||
} | |||
|
|||
// TODO: review if can remove this code |
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.
resolved: dead code
@@ -275,7 +275,6 @@ func getResourceNameByID(resourceID string) string { | |||
|
|||
// generateStorageProfile generates a pointer to a compute.StorageProfile which can utilized for VM creation. | |||
func generateStorageProfile(vmSpec Spec) (*compute.StorageProfile, error) { | |||
// TODO: Validate parameters before building storage profile |
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.
@@ -89,7 +89,6 @@ func (s *Service) Reconcile(ctx context.Context, spec interface{}) error { | |||
s.Scope.V(2).Info("Working on custom vnet", "vnet-id", vnet.ID) | |||
} | |||
// vnet already exists, cannot update since it's immutable | |||
// TODO: ensure tags & other managed vnet attributes |
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.
resolved: this is not needed: if the vnet is detected as "managed" then it already has the "owner" tags. If not, then it is probably unmanaged and we shouldn't apply those tags. I don't think "other managed vnet attributes" applies here, I believe this is a leftover copy paste from capa.
@@ -123,8 +123,6 @@ func (r *AzureClusterReconciler) reconcileNormal(clusterScope *scope.ClusterScop | |||
return reconcile.Result{}, errors.Wrap(err, "failed to reconcile cluster services") | |||
} | |||
|
|||
// TODO: We may need to use azureCluster.Status.Network.APIServerIP.IPAddress |
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.
@@ -217,7 +217,6 @@ func (r *AzureMachineReconciler) reconcileNormal(ctx context.Context, machineSco | |||
return reconcile.Result{}, err | |||
} | |||
|
|||
// TODO(ncdc): move this validation logic into a validating webhook |
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.
// validateUpdate checks that no immutable fields have been updated and | ||
// returns a slice of errors representing attempts to change immutable state. | ||
func (r *AzureMachineReconciler) validateUpdate(spec *infrav1.AzureMachineSpec, i *infrav1.VM) (errs []error) { | ||
// TODO: Add comparison logic for immutable fields |
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.
// Make sure Spec.ProviderID is always set. | ||
machineScope.SetProviderID(fmt.Sprintf("azure:////%s", vm.ID)) | ||
|
||
// Proceed to reconcile the AzureMachine state. | ||
machineScope.SetVMState(vm.State) | ||
|
||
// TODO(vincepri): Remove this annotation when clusterctl is no longer relevant. |
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.
@vincepri is this still relevant?
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.
I don't think we use this any longer, cc @fabriziopandini
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.
clusterctl uses cluster.x-k8s.io/provider
label, so AFAIK this annotation can be removed
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.
hmm I removed it and now E2E is failing so maybe it is needed...
@@ -42,7 +42,6 @@ const ( | |||
) | |||
|
|||
// azureMachineService are list of services required by cluster actuator, easy to create a fake | |||
// TODO: We should decide if we want to keep this |
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.
@@ -24,17 +24,3 @@ require ( | |||
sigs.k8s.io/kustomize/kustomize/v3 v3.5.4 | |||
sigs.k8s.io/testing_frameworks v0.1.2 | |||
) | |||
|
|||
replace ( | |||
// TODO(vincepri) Remove the following replace directives, needed for golangci-lint until |
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.
resolved: golangci/golangci-lint#595 is fixed.
@@ -133,7 +133,6 @@ run_tests() { | |||
} | |||
|
|||
get_logs() { | |||
# TODO collect more logs https://github.com/kubernetes-sigs/cluster-api-provider-azure/issues/474 |
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.
@@ -49,7 +49,6 @@ func (g *ClusterAPI) releaseYAMLPath() string { | |||
|
|||
// Manifests return the generated components and any error if there is one. | |||
func (g *ClusterAPI) Manifests(ctx context.Context) ([]byte, error) { | |||
// TODO: this is not very nice |
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.
@jadarsie is this still relevant? If so could you please file an issue with what you had in mind
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.
Not my comment actually, it was there in the first iteration of the test framework
kubernetes-sigs/cluster-api@ea48d0c#diff-c4cddf335f3f881e1b942982a3ada01f
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.
thanks, removed those files in #569 because I realized these were duplicates from the ones in the upstream framework.
@@ -59,6 +59,5 @@ func expandCredVariables(stdout []byte, creds auth.Creds) []byte { | |||
os.Setenv("AZURE_CLIENT_SECRET_B64", base64.StdEncoding.EncodeToString([]byte(creds.ClientSecret))) | |||
os.Setenv("AZURE_SUBSCRIPTION_ID_B64", base64.StdEncoding.EncodeToString([]byte(creds.SubscriptionID))) | |||
os.Setenv("AZURE_TENANT_ID_B64", base64.StdEncoding.EncodeToString([]byte(creds.TenantID))) | |||
// TODO Figure out a better way of doing this |
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.
@jadarsie same here
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.
I do not remember what I meant. Probably something related to the fact that stdout
is turned into string
and []byte
back and forth a few times.
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.
I think it can be removed
@@ -47,7 +47,4 @@ var _ = Describe("Metacluster", func() { | |||
AfterEach(func() { | |||
kindCluster.Teardown() | |||
}) | |||
|
|||
// TODO: validate that the controller-manager is deployed and the |
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.
resolved: this test suite is empty and not being used. @justaugustus to confirm there is nothing to do here.
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.
Fine to remove
/hold waiting for a few responses from TODO items owners |
efbe06a
to
13ca645
Compare
/hold cancel |
/retest |
/hold cancel |
/retest |
8258cb5
to
d6b045d
Compare
d6b045d
to
9734e9f
Compare
This is a really impressive cleanup, @CecileRobertMichon! |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CecileRobertMichon, justaugustus 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 |
What this PR does / why we need it: Removed all
TODO
s from the code and either 1) Opened a corresponding issue 2) marked as resolve.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 #148
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: