-
Notifications
You must be signed in to change notification settings - Fork 413
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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. |
||
|
||
// TODO: Write type tests | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved: there are no testable functions in this file, only type definitions |
||
|
||
// AzureResourceReference is a reference to a specific Azure resource by ID | ||
type AzureResourceReference struct { | ||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: removed |
||
} | ||
|
||
// TODO: Investigate resource filters | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
|
||
// AzureMachineProviderConditionType is a valid value for AzureMachineProviderCondition.Type | ||
type AzureMachineProviderConditionType string | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: dead code |
||
// String returns a string representation of the security group. | ||
func (s *SecurityGroup) String() string { | ||
return fmt.Sprintf("id=%s/name=%s", s.ID, s.Name) | ||
} | ||
*/ | ||
|
||
// SecurityGroupProtocol defines the protocol type for a security group rule. | ||
type SecurityGroupProtocol string | ||
|
||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: dead code |
||
// String returns a string representation of the ingress rule. | ||
/* | ||
func (i *IngressRule) String() string { | ||
return fmt.Sprintf("protocol=%s/range=[%d-%d]/description=%s", i.Protocol, i.FromPort, i.ToPort, i.Description) | ||
} | ||
*/ | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: dead code |
||
// Difference returns the difference between this slice and the other slice. | ||
/* | ||
func (i IngressRules) Difference(o IngressRules) (out IngressRules) { | ||
for _, x := range i { | ||
found := false | ||
for _, y := range o { | ||
sort.Strings(x.CidrBlocks) | ||
sort.Strings(y.CidrBlocks) | ||
sort.Strings(x.SourceSecurityGroupIDs) | ||
sort.Strings(y.SourceSecurityGroupIDs) | ||
if reflect.DeepEqual(x, y) { | ||
found = true | ||
break | ||
} | ||
} | ||
if !found { | ||
out = append(out, x) | ||
} | ||
} | ||
return | ||
} | ||
*/ | ||
|
||
// 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 commentThe 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 |
||
type PublicIP struct { | ||
ID string `json:"id,omitempty"` | ||
Name string `json:"name,omitempty"` | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// subnet already exists, skip creation | ||
if subnetSpec.Role == infrav1.SubnetControlPlane { | ||
subnet.DeepCopyInto(s.Scope.ControlPlaneSubnet()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: dead code |
||
// if *vmExt.ProvisioningState != string(compute.ProvisioningStateSucceeded) { | ||
// // If the script failed delete it so it can be retried | ||
// s.Delete(ctx, vmExtSpec) | ||
// } | ||
|
||
klog.V(2).Infof("successfully created vm extension %s ", vmExtSpec.Name) | ||
return nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
storageProfile := &compute.StorageProfile{ | ||
OsDisk: &compute.OSDisk{ | ||
Name: to.StringPtr(azure.GenerateOSDiskName(vmSpec.Name)), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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. |
||
vnet.DeepCopyInto(s.Scope.Vnet()) | ||
return nil | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
// instead when we look at configuring private clusters. | ||
if azureCluster.Status.Network.APIServerIP.DNSName == "" { | ||
clusterScope.Info("Waiting for API server endpoint to exist") | ||
return reconcile.Result{RequeueAfter: 15 * time.Second}, nil | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -217,24 +217,16 @@ 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if errs := r.validateUpdate(&machineScope.AzureMachine.Spec, vm); len(errs) > 0 { | ||
agg := kerrors.NewAggregate(errs) | ||
r.Recorder.Eventf(machineScope.AzureMachine, corev1.EventTypeWarning, "InvalidUpdate", "Invalid update: %s", agg.Error()) | ||
return reconcile.Result{}, nil | ||
} | ||
|
||
// 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. clusterctl uses There was a problem hiding this comment. Choose a reason for hiding this commentThe 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... |
||
machineScope.SetAnnotation("cluster-api-provider-azure", "true") | ||
|
||
machineScope.SetAddresses(vm.Addresses) | ||
|
||
// Proceed to reconcile the AzureMachine state. | ||
machineScope.SetVMState(vm.State) | ||
|
||
switch vm.State { | ||
case infrav1.VMStateSucceeded: | ||
machineScope.V(2).Info("VM is running", "id", *machineScope.GetVMID()) | ||
|
@@ -306,14 +298,7 @@ func (r *AzureMachineReconciler) reconcileDelete(machineScope *scope.MachineScop | |
return reconcile.Result{}, nil | ||
} | ||
|
||
// 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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
return errs | ||
} | ||
|
||
// AzureClusterToAzureMachine is a handler.ToRequestsFunc to be used to enqueue requests for reconciliation | ||
// AzureClusterToAzureMachines is a handler.ToRequestsFunc to be used to enqueue requests for reconciliation | ||
// of AzureMachines. | ||
func (r *AzureMachineReconciler) AzureClusterToAzureMachines(o handler.MapObject) []ctrl.Request { | ||
result := []ctrl.Request{} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
type azureMachineService struct { | ||
machineScope *scope.MachineScope | ||
clusterScope *scope.ClusterScope | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. resolved: golangci/golangci-lint#595 is fixed. |
||
// https://github.com/golangci/golangci-lint/issues/595 is fixed. | ||
github.com/go-critic/go-critic v0.0.0-20181204210945-1df300866540 => github.com/go-critic/go-critic v0.0.0-20190526074819-1df300866540 | ||
github.com/golangci/errcheck v0.0.0-20181003203344-ef45e06d44b6 => github.com/golangci/errcheck v0.0.0-20181223084120-ef45e06d44b6 | ||
github.com/golangci/go-tools v0.0.0-20180109140146-af6baa5dc196 => github.com/golangci/go-tools v0.0.0-20190318060251-af6baa5dc196 | ||
github.com/golangci/gofmt v0.0.0-20181105071733-0b8337e80d98 => github.com/golangci/gofmt v0.0.0-20181222123516-0b8337e80d98 | ||
github.com/golangci/gosec v0.0.0-20180901114220-66fb7fc33547 => github.com/golangci/gosec v0.0.0-20190211064107-66fb7fc33547 | ||
github.com/golangci/ineffassign v0.0.0-20180808204949-42439a7714cc => github.com/golangci/ineffassign v0.0.0-20190609212857-42439a7714cc | ||
github.com/golangci/lint-1 v0.0.0-20180610141402-ee948d087217 => github.com/golangci/lint-1 v0.0.0-20190420132249-ee948d087217 | ||
github.com/timakin/bodyclose => github.com/golangci/bodyclose v0.0.0-20190714144026-65da19158fa2 | ||
mvdan.cc/unparam v0.0.0-20190124213536-fbb59629db34 => mvdan.cc/unparam v0.0.0-20190209190245-fbb59629db34 | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -124,7 +124,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 commentThe reason will be displayed to describe this comment to others. Learn more. |
||
kubectl logs deploy/capz-controller-manager -n capz-system manager > "${ARTIFACTS}/logs/capz-manager.log" || true | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 commentThe 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. |
||
if g.GitRef != "" { | ||
kustomize := exec.NewCommand( | ||
exec.WithCommand("kustomize"), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it can be removed |
||
return []byte(os.ExpandEnv(string(stdout))) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Fine to remove |
||
// types are available | ||
}) |
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.
#554