Replace TO-DOs with FIXMEs (nginx#95)

- Replace all TO-DOs with FIXMEs to follow our coding guidelines
(TO-DOs -- actually TODOs -- must be addressed and removed before
merging a PR)
- Improve some messages for clarity
- Remove unnecessary TO-DOs

Author: miledxz

cmd/gateway/main.go

@@ -40,7 +40,7 @@ func main() {
 
 	MustValidateArguments(
 		flag.CommandLine,
-		GatewayControllerParam(domain, "nginx-gateway" /* TODO dynamically set */),
+		GatewayControllerParam(domain, "nginx-gateway" /* FIXME(f5yacobucci) dynamically set */),
 	)
 
 	logger.Info("Starting NGINX Kubernetes Gateway",

internal/events/loop.go

@@ -68,7 +68,7 @@ func (el *EventLoop) Start(ctx context.Context) error {
 	}
 }
 
-// TO-DO: think about how to avoid using an interface{} here
+// FIXME(pleshakov): think about how to avoid using an interface{} here
 func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) error {
 	var changes []state.Change
 	var updates []state.StatusUpdate
@@ -80,7 +80,7 @@ func (el *EventLoop) handleEvent(ctx context.Context, event interface{}) error {
 	case *DeleteEvent:
 		changes, updates, err = el.propagateDelete(e)
 	default:
-		// TO-DO: panic
+		// FIXME(pleshakov): panic because it is a coding error
 		return fmt.Errorf("unknown event type %T", e)
 	}
 
@@ -99,11 +99,11 @@ func (el *EventLoop) propagateUpsert(e *UpsertEvent) ([]state.Change, []state.St
 		return changes, statusUpdates, nil
 	case *apiv1.Service:
 		el.serviceStore.Upsert(r)
-		// TO-DO: make sure the affected hosts are updated
+		// FIXME(pleshakov): make sure the affected hosts are updated
 		return nil, nil, nil
 	}
 
-	// TO-DO: panic
+	// FIXME(pleshakov): panic because it is a coding error
 	return nil, nil, fmt.Errorf("unknown resource type %T", e.Resource)
 }
 
@@ -114,11 +114,11 @@ func (el *EventLoop) propagateDelete(e *DeleteEvent) ([]state.Change, []state.St
 		return changes, statusUpdates, nil
 	case *apiv1.Service:
 		el.serviceStore.Delete(e.NamespacedName)
-		// TO-DO: make sure the affected hosts are updated
+		// FIXME(pleshakov): make sure the affected hosts are updated
 		return nil, nil, nil
 	}
 
-	// TO-DO: panic
+	// FIXME(pleshakov): panic because it is a coding error
 	return nil, nil, fmt.Errorf("unknown resource type %T", e.Type)
 }
 
@@ -132,7 +132,7 @@ func (el *EventLoop) processChangesAndStatusUpdates(ctx context.Context, changes
 
 			for obj, objWarnings := range warnings {
 				for _, w := range objWarnings {
-					// TO-DO: report warnings via Object status
+					// FIXME(pleshakov): report warnings via Object status
 					el.logger.Info("got warning while generating config",
 						"kind", obj.GetObjectKind().GroupVersionKind().Kind,
 						"namespace", obj.GetNamespace(),

internal/implementations/service/service.go

@@ -15,7 +15,7 @@ type serviceImplementation struct {
 	eventCh chan<- interface{}
 }
 
-// TO-DO: serviceImplementation looks similar to httpRouteImplemenation
+// FIXME(pleshakov): serviceImplementation looks similar to httpRouteImplemenation
 // consider if it is possible to reduce the amount of code.
 
 // NewServiceImplementation creates a new ServiceImplementation.

internal/manager/manager.go

@@ -28,7 +28,7 @@ const clusterTimeout = 10 * time.Second
 var scheme = runtime.NewScheme()
 
 func init() {
-	// TO-DO: handle errors returned by the calls bellow
+	// FIXME(pleshakov): handle errors returned by the calls bellow
 	_ = gatewayv1alpha2.AddToScheme(scheme)
 	_ = apiv1.AddToScheme(scheme)
 }

internal/nginx/config/generator.go

@@ -43,12 +43,12 @@ func (g *GeneratorImpl) GenerateForHost(host state.Host) ([]byte, Warnings) {
 func generate(host state.Host, serviceStore state.ServiceStore) (server, Warnings) {
 	warnings := newWarnings()
 
-	locs := make([]location, 0, len(host.PathRouteGroups)) // TO-DO: expand with g.Routes
+	locs := make([]location, 0, len(host.PathRouteGroups)) // FIXME(pleshakov): expand with g.Routes
 
 	for _, g := range host.PathRouteGroups {
 		// number of routes in a group is always at least 1
 		// otherwise, it is a bug in the state.Configuration code, so it is OK to panic here
-		r := g.Routes[0] // TO-DO: for now, we only handle the first route in case there are multiple routes
+		r := g.Routes[0] // FIXME(pleshakov): for now, we only handle the first route in case there are multiple routes
 		address, err := getBackendAddress(r.Source.Spec.Rules[r.RuleIdx].BackendRefs, r.Source.Namespace, serviceStore)
 		if err != nil {
 			warnings.AddWarning(r.Source, err.Error())
@@ -75,12 +75,11 @@ func generateProxyPass(address string) string {
 }
 
 func getBackendAddress(refs []v1alpha2.HTTPBackendRef, parentNS string, serviceStore state.ServiceStore) (string, error) {
-	// TO-DO: make sure the warnings are generated and reported to the user fot the edge cases
 	if len(refs) == 0 {
 		return "", errors.New("empty backend refs")
 	}
 
-	// TO-DO: for now, we only support a single backend reference
+	// FIXME(pleshakov): for now, we only support a single backend reference
 	ref := refs[0].BackendRef
 
 	if ref.Kind != nil && *ref.Kind != "Service" {

internal/nginx/config/template.go

@@ -20,8 +20,8 @@ var serverTemplate = `server {
 
 // templateExecutor generates NGINX configuration using a template.
 // Template parsing or executing errors can only occur if there is a bug in the template, so they are handled with panics.
-// TO-DO: for now, we only generate configuration with NGINX server.
-// We will also need to generate the main NGINX configuration file, upstreams.
+// For now, we only generate configuration with NGINX server, but in the future we will also need to generate
+// the main NGINX configuration file, upstreams.
 type templateExecutor struct {
 	serverTemplate *template.Template
 }

internal/state/configuration.go

@@ -231,7 +231,7 @@ func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) {
 
 	statusUpdates := make([]StatusUpdate, 0, len(listener.httpRoutes))
 
-	// TO-DO: optimize it so that we only update the status of the affected (changed) httpRoutes
+	// FIXME(pleshakov): optimize it so that we only update the status of the affected (changed) httpRoutes
 	// getSortedKeys is used to ensure predictable order for unit tests
 	for _, key := range getSortedKeys(listener.httpRoutes) {
 		route := listener.httpRoutes[key]
@@ -242,7 +242,7 @@ func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) {
 					Parents: []v1alpha2.RouteParentStatus{
 						{
 							ParentRef: v1alpha2.ParentRef{
-								Name: "fake", // TO-DO: report the parent ref properly
+								Name: "fake", // FIXME(pleshakov): report the parent ref properly
 							},
 							ControllerName: v1alpha2.GatewayController(c.gatewayCtlrName),
 							Conditions: []metav1.Condition{
@@ -252,7 +252,7 @@ func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) {
 									ObservedGeneration: listener.httpRoutes[key].Generation,
 									LastTransitionTime: metav1.NewTime(c.clock.Now()),
 									Reason:             string(v1alpha2.ConditionRouteAccepted),
-									Message:            "", // TO-DO: figure out a good message
+									Message:            "", // FIXME(pleshakov): figure out a good message
 								},
 							},
 						},
@@ -263,7 +263,7 @@ func (c *configurationImpl) updateListeners() ([]Change, []StatusUpdate) {
 		statusUpdates = append(statusUpdates, update)
 	}
 
-	// TO-DO: remove the accepted condition for the excluded (no longer handled) Routes
+	// FIXME(pleshakov): remove the accepted condition for the excluded (no longer handled) Routes
 
 	return changes, statusUpdates
 }
@@ -318,7 +318,7 @@ func createChanges(removedHosts []string, updatedHosts []string, addedHosts []st
 func determineChangesInHosts(listener httpListener, newHosts hosts) (removedHosts []string, updatedHosts []string, addedHosts []string) {
 	// getSortedKeys is used to ensure predictable order for unit tests
 
-	// TO-DO: consider using a data structure for sets
+	// FIXME(pleshakov): consider using a data structure for sets
 
 	for _, h := range getSortedKeys(listener.hosts) {
 		_, exists := newHosts[h]

internal/state/services.go

@@ -17,7 +17,7 @@ type ServiceStore interface {
 	Delete(nsname types.NamespacedName)
 	// Resolve returns the cluster IP  the service specified by its namespace and name.
 	// If the service doesn't have a cluster IP or it doesn't exist, resolve will return an error.
-	// TO-DO: later, we will start using the Endpoints rather than cluster IPs.
+	// FIXME(pleshakov): later, we will start using the Endpoints rather than cluster IPs.
 	Resolve(nsname types.NamespacedName) (string, error)
 }
 

internal/status/updater.go

@@ -27,24 +27,24 @@ type Updater interface {
 //
 // (1) It doesn't understand the leader election. Only the leader must report the statuses of the resources. Otherwise,
 // multiple replicas will step on each other when trying to report statuses for the same resources.
-// TO-DO: address limitation (1)
+// FIXME(pleshakov): address limitation (1)
 //
 // (2) It is synchronous, which means the status reporter can slow down the event loop.
 // Consider the following cases:
 // (a) Sometimes the Gateway will need to update statuses of all resources it handles, which could be ~1000. Making 1000
 // status API calls sequentially will take time.
 // (b) k8s API can become slow or even timeout. This will increase every update status API call.
 // Making updaterImpl asynchronous will prevent it from adding variable delays to the event loop.
-// TO-DO: address limitation (2)
+// FIXME(pleshakov) address limitation (2)
 //
 // (3) It doesn't retry on failures. This means there is a chance that some resources will not have up-to-do statuses.
 // Statuses are important part of the Gateway API, so we need to ensure that the Gateway always keep the resources
 // statuses up-to-date.
-// TO-DO: address limitation (3)
+// FIXME(pleshakov): address limitation (3)
 //
 // (4) To support new resources, updaterImpl needs to be modified. Consider making updaterImpl extendable, so that it
 // goes along the Open-closed principle.
-// TO-DO: address limitation (4)
+// FIXME(pleshakov): address limitation (4)
 type updaterImpl struct {
 	client client.Client
 	logger logr.Logger
@@ -77,7 +77,7 @@ func (upd *updaterImpl) ProcessStatusUpdates(ctx context.Context, updates []stat
 
 			upd.update(ctx, u.NamespacedName, &hr, func(object client.Object) {
 				route := object.(*v1alpha2.HTTPRoute)
-				// TO-DO: merge the conditions in the status with the conditions in the route.Status properly, because
+				// FIXME(pleshakov): merge the conditions in the status with the conditions in the route.Status properly, because
 				// right now, we are replacing the conditions.
 				route.Status = *s
 			})
@@ -89,7 +89,7 @@ func (upd *updaterImpl) ProcessStatusUpdates(ctx context.Context, updates []stat
 
 func (upd *updaterImpl) update(ctx context.Context, nsname types.NamespacedName, obj client.Object, statusSetter func(client.Object)) {
 	// The function handles errors by reporting them in the logs.
-	// TO-DO: figure out appropriate log level for these errors. Perhaps 3?
+	// FIXME(pleshakov): figure out appropriate log level for these errors. Perhaps 3?
 
 	// We need to get the latest version of the resource.
 	// Otherwise, the Update status API call can fail.

pkg/sdk/interfaces.go

@@ -25,7 +25,7 @@ type GatewayConfigImpl interface {
 
 type HTTPRouteImpl interface {
 	Upsert(config *v1alpha2.HTTPRoute)
-	// TO-DO: change other interfaces to use types.NamespacedName
+	// FIXME(pleshakov): change other interfaces to use types.NamespacedName
 	Remove(types.NamespacedName)
 }