Skip to content

Commit 9fde532

Browse files
cbumbcbumb
committed
fix: Clean up nolint directives marked as TODO - Part 6
1 parent d2dead6 commit 9fde532

File tree

9 files changed

+517
-485
lines changed

9 files changed

+517
-485
lines changed

‎janitor-provider/pkg/csp/gcp/gcp.go‎

Lines changed: 29 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -105,14 +105,8 @@ func getNodeFields(node corev1.Node) (*gcpNodeFields, error) {
105105
}
106106

107107
// SendRebootSignal resets a GCE node by stopping and starting the instance.
108-
// nolint:dupl // Similar code pattern as SendTerminateSignal is expected for CSP operations
109108
func (c *Client) SendRebootSignal(ctx context.Context, node corev1.Node) (model.ResetSignalRequestRef, error) {
110-
httpClient, err := getAuthenticatedHTTPClient(ctx)
111-
if err != nil {
112-
return "", err
113-
}
114-
115-
instancesClient, err := compute.NewInstancesRESTClient(ctx, option.WithHTTPClient(httpClient))
109+
instancesClient, nodeFields, err := prepareInstanceOp(ctx, node)
116110
if err != nil {
117111
return "", err
118112
}
@@ -123,11 +117,6 @@ func (c *Client) SendRebootSignal(ctx context.Context, node corev1.Node) (model.
123117
}
124118
}()
125119

126-
nodeFields, err := getNodeFields(node)
127-
if err != nil {
128-
return "", err
129-
}
130-
131120
resetReq := &computepb.ResetInstanceRequest{
132121
Instance: nodeFields.instance,
133122
Project: nodeFields.project,
@@ -138,7 +127,7 @@ func (c *Client) SendRebootSignal(ctx context.Context, node corev1.Node) (model.
138127

139128
op, err := instancesClient.Reset(ctx, resetReq)
140129
if err != nil {
141-
return "", err
130+
return "", fmt.Errorf("reset instance %s: %w", nodeFields.instance, err)
142131
}
143132

144133
return model.ResetSignalRequestRef(op.Proto().GetName()), nil
@@ -186,14 +175,8 @@ func (c *Client) IsNodeReady(ctx context.Context, node corev1.Node, requestID st
186175
}
187176

188177
// SendTerminateSignal deletes a GCE node.
189-
// nolint:dupl // Similar code pattern as SendRebootSignal is expected for CSP operations
190178
func (c *Client) SendTerminateSignal(ctx context.Context, node corev1.Node) (model.TerminateNodeRequestRef, error) {
191-
httpClient, err := getAuthenticatedHTTPClient(ctx)
192-
if err != nil {
193-
return "", err
194-
}
195-
196-
instancesClient, err := compute.NewInstancesRESTClient(ctx, option.WithHTTPClient(httpClient))
179+
instancesClient, nodeFields, err := prepareInstanceOp(ctx, node)
197180
if err != nil {
198181
return "", err
199182
}
@@ -204,11 +187,6 @@ func (c *Client) SendTerminateSignal(ctx context.Context, node corev1.Node) (mod
204187
}
205188
}()
206189

207-
nodeFields, err := getNodeFields(node)
208-
if err != nil {
209-
return "", err
210-
}
211-
212190
deleteReq := &computepb.DeleteInstanceRequest{
213191
Instance: nodeFields.instance,
214192
Project: nodeFields.project,
@@ -219,8 +197,33 @@ func (c *Client) SendTerminateSignal(ctx context.Context, node corev1.Node) (mod
219197

220198
op, err := instancesClient.Delete(ctx, deleteReq)
221199
if err != nil {
222-
return "", err
200+
return "", fmt.Errorf("delete instance %s: %w", nodeFields.instance, err)
223201
}
224202

225203
return model.TerminateNodeRequestRef(op.Proto().GetName()), nil
226204
}
205+
206+
func prepareInstanceOp(
207+
ctx context.Context, node corev1.Node,
208+
) (*compute.InstancesClient, *gcpNodeFields, error) {
209+
httpClient, err := getAuthenticatedHTTPClient(ctx)
210+
if err != nil {
211+
return nil, nil, fmt.Errorf("failed to create authenticated HTTP client: %w", err)
212+
}
213+
214+
instancesClient, err := compute.NewInstancesRESTClient(ctx, option.WithHTTPClient(httpClient))
215+
if err != nil {
216+
return nil, nil, fmt.Errorf("failed to create instances client: %w", err)
217+
}
218+
219+
nodeFields, err := getNodeFields(node)
220+
if err != nil {
221+
if cerr := instancesClient.Close(); cerr != nil {
222+
slog.Error("failed to close instances client", "error", cerr)
223+
}
224+
225+
return nil, nil, fmt.Errorf("failed to get node fields for node %q: %w", node.Name, err)
226+
}
227+
228+
return instancesClient, nodeFields, nil
229+
}

‎janitor-provider/pkg/csp/kind/kind.go‎

Lines changed: 57 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ func (c *Client) IsNodeReady(ctx context.Context, node corev1.Node, requestID st
6161
}
6262

6363
// SendTerminateSignal simulates terminating a kind node by removing the docker container
64-
// nolint:funlen,gocyclo,cyclop // Complex docker interaction logic
6564
func (c *Client) SendTerminateSignal(
6665
ctx context.Context,
6766
node corev1.Node,
@@ -108,53 +107,71 @@ func (c *Client) SendTerminateSignal(
108107
return "", fmt.Errorf("failed to list containers: %w", err)
109108
}
110109

111-
// nolint:nestif // Complex docker interaction logic migrated from old code
112-
// If container exists, delete it
113-
if strings.Contains(string(output), containerName) {
114-
slog.Info("Found container, attempting deletion", "container", containerName)
110+
found := false
115111

116-
// nolint:gosec // G204: Command args are derived from kubernetes API, not user input
117-
cmd = exec.CommandContext(dockerCtx, "docker", "rm", "-f", containerName)
112+
for _, line := range strings.Split(strings.TrimSpace(string(output)), "\n") {
113+
if line == containerName {
114+
found = true
115+
break
116+
}
117+
}
118118

119-
if err := cmd.Run(); err != nil {
120-
if ctx.Err() == context.DeadlineExceeded {
121-
return "", fmt.Errorf("timeout while deleting container: %w", err)
122-
}
119+
if !found {
120+
slog.Info("Container not found, assuming already deleted", "container", containerName)
123121

124-
return "", fmt.Errorf("failed to delete container: %w", err)
125-
}
122+
return model.TerminateNodeRequestRef(""), nil
123+
}
124+
125+
slog.Info("Found container, attempting deletion", "container", containerName)
126+
127+
if err := c.deleteAndVerifyContainer(ctx, dockerCtx, containerName); err != nil {
128+
return "", err
129+
}
126130

127-
// Verify container is actually gone
128-
// nolint:gosec // G204: Command args are derived from kubernetes API, not user input
129-
cmd = exec.CommandContext(
130-
dockerCtx,
131-
"docker",
132-
"ps",
133-
"-a",
134-
"--filter",
135-
fmt.Sprintf("name=^%s$", containerName),
136-
"--format",
137-
"{{.Names}}",
138-
)
139-
140-
output, err = cmd.Output()
141-
if err != nil {
142-
if ctx.Err() == context.DeadlineExceeded {
143-
return "", fmt.Errorf("timeout while verifying container deletion: %w", err)
144-
}
145-
146-
return "", fmt.Errorf("failed to verify container deletion: %w", err)
131+
slog.Info("Successfully deleted container", "container", containerName)
132+
133+
return model.TerminateNodeRequestRef(""), nil
134+
}
135+
136+
func (c *Client) deleteAndVerifyContainer(
137+
ctx, dockerCtx context.Context, containerName string,
138+
) error {
139+
// nolint:gosec // G204: Command args are derived from kubernetes API, not user input
140+
cmd := exec.CommandContext(dockerCtx, "docker", "rm", "-f", containerName)
141+
142+
if err := cmd.Run(); err != nil {
143+
if dockerCtx.Err() == context.DeadlineExceeded {
144+
return fmt.Errorf("timeout while deleting container: %w", err)
147145
}
148146

149-
if strings.Contains(string(output), containerName) {
150-
return model.TerminateNodeRequestRef(""),
151-
fmt.Errorf("container %s still exists after deletion attempt", containerName)
147+
return fmt.Errorf("failed to delete container: %w", err)
148+
}
149+
150+
// Verify container is actually gone
151+
// nolint:gosec // G204: Command args are derived from kubernetes API, not user input
152+
cmd = exec.CommandContext(
153+
dockerCtx,
154+
"docker",
155+
"ps",
156+
"-a",
157+
"--filter",
158+
fmt.Sprintf("name=^%s$", containerName),
159+
"--format",
160+
"{{.Names}}",
161+
)
162+
163+
output, err := cmd.Output()
164+
if err != nil {
165+
if dockerCtx.Err() == context.DeadlineExceeded {
166+
return fmt.Errorf("timeout while verifying container deletion: %w", err)
152167
}
153168

154-
slog.Info("Successfully deleted container", "container", containerName)
155-
} else {
156-
slog.Info("Container not found, assuming already deleted", "container", containerName)
169+
return fmt.Errorf("failed to verify container deletion: %w", err)
157170
}
158171

159-
return model.TerminateNodeRequestRef(""), nil
172+
if strings.Contains(string(output), containerName) {
173+
return fmt.Errorf("container %s still exists after deletion attempt", containerName)
174+
}
175+
176+
return nil
160177
}

‎store-client/pkg/adapter/legacy_adapter.go‎

Lines changed: 28 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"fmt"
1919
"os"
2020
"path/filepath"
21+
"strings"
2122

2223
"github.com/nvidia/nvsentinel/store-client/pkg/config"
2324
"github.com/nvidia/nvsentinel/store-client/pkg/datastore"
@@ -90,61 +91,41 @@ func (l *LegacyDatabaseConfigAdapter) GetCollectionName() string {
9091
}
9192

9293
// buildPostgreSQLConnectionString builds a PostgreSQL connection string from DataStoreConfig
93-
//
94-
//nolint:cyclop // Complex but clear connection string building logic
9594
func (l *LegacyDatabaseConfigAdapter) buildPostgreSQLConnectionString() string {
9695
conn := l.dsConfig.Connection
97-
params := []string{}
98-
99-
if conn.Host != "" {
100-
params = append(params, "host="+conn.Host)
101-
}
102-
103-
if conn.Port > 0 {
104-
params = append(params, fmt.Sprintf("port=%d", conn.Port))
105-
}
106-
107-
if conn.Database != "" {
108-
params = append(params, "dbname="+conn.Database)
109-
}
110-
111-
if conn.Username != "" {
112-
params = append(params, "user="+conn.Username)
113-
}
114-
115-
if conn.Password != "" {
116-
params = append(params, "password="+conn.Password)
117-
}
11896

119-
if conn.SSLMode != "" {
120-
params = append(params, "sslmode="+conn.SSLMode)
121-
}
122-
123-
if conn.SSLCert != "" {
124-
params = append(params, "sslcert="+conn.SSLCert)
125-
}
126-
127-
if conn.SSLKey != "" {
128-
params = append(params, "sslkey="+conn.SSLKey)
129-
}
130-
131-
if conn.SSLRootCert != "" {
132-
params = append(params, "sslrootcert="+conn.SSLRootCert)
97+
candidates := []struct {
98+
key string
99+
value string
100+
}{
101+
{"host", conn.Host},
102+
{"port", formatPort(conn.Port)},
103+
{"dbname", conn.Database},
104+
{"user", conn.Username},
105+
{"password", conn.Password},
106+
{"sslmode", conn.SSLMode},
107+
{"sslcert", conn.SSLCert},
108+
{"sslkey", conn.SSLKey},
109+
{"sslrootcert", conn.SSLRootCert},
110+
}
111+
112+
var params []string
113+
114+
for _, c := range candidates {
115+
if c.value != "" {
116+
params = append(params, c.key+"="+c.value)
117+
}
133118
}
134119

135-
// Join all parameters with spaces
136-
137-
result := ""
138-
139-
for i, param := range params {
140-
if i > 0 {
141-
result += " "
142-
}
120+
return strings.Join(params, " ")
121+
}
143122

144-
result += param
123+
func formatPort(port int) string {
124+
if port > 0 {
125+
return fmt.Sprintf("%d", port)
145126
}
146127

147-
return result
128+
return ""
148129
}
149130

150131
func (l *LegacyDatabaseConfigAdapter) GetCertConfig() config.CertificateConfig {

0 commit comments

Comments
 (0)