Skip to content

Replace string matching with JSON parsing in isRemoteWorkload - issue - 2941 #2968

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

Open
deepika1214 wants to merge 9 commits into
base: main
Choose a base branch
from
+67 −6
Open

Replace string matching with JSON parsing in isRemoteWorkload - issue - 2941 #2968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8dd06ee
Replace string matching with JSON parsing in isRemoteWorkload - issue…
deepika1214 Dec 9, 2025
411ba02
Merge branch 'main' into issue-2941
deepika1214 Dec 9, 2025
aad19ae
fixed lint issues
deepika1214 Dec 9, 2025
e7bd60e
Merge branch 'main' into issue-2941
deepika1214 Dec 10, 2025
207c945
Merge branch 'main' into issue-2941
deepika1214 Dec 10, 2025
f9de1f3
Merge branch 'main' into issue-2941
jhrozek Dec 11, 2025
7cb1318
Merge branch 'main' into issue-2941
deepika1214 Dec 13, 2025
159eb7b
Merge branch 'main' into issue-2941
deepika1214 Dec 18, 2025
a86f4f9
Merge branch 'main' into issue-2941
eleftherias Dec 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions pkg/workloads/statuses/file_status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"fmt"
"io"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -94,14 +93,17 @@ func (f *fileStatusManager) isRemoteWorkload(ctx context.Context, workloadName s
}
defer reader.Close()

// Read the configuration data
data, err := io.ReadAll(reader)
if err != nil {
// Parse the JSON to check for remote_url field
var config struct {
RemoteURL string `json:"remote_url"`
}
decoder := json.NewDecoder(reader)
if err := decoder.Decode(&config); err != nil {
return false, err
}

// Check if the JSON contains "remote_url" field
return strings.Contains(string(data), `"remote_url"`), nil
// Check if the remote_url field is set
return strings.TrimSpace(config.RemoteURL) != "", nil
}

// remoteWorkloadConfig is a minimal struct to parse only the fields we need from RunConfig
Expand Down
59 changes: 59 additions & 0 deletions pkg/workloads/statuses/file_status_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1867,3 +1867,62 @@ func TestFileStatusManager_ListWorkloads_PIDMigration(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, existingPID, statusFile2.ProcessID, "PID should remain unchanged for second workload")
}

func TestFileStatusManager_IsRemoteWorkload_EdgeCases(t *testing.T) {
t.Parallel()

tests := []struct {
name string
configJSON string
expected bool
}{
{
name: "remote workload with URL",
configJSON: `{"remote_url": "https://example.com"}`,
expected: true,
},
{
name: "local workload without remote_url field",
configJSON: `{"name": "test-workload"}`,
expected: false,
},
{
name: "edge case - remote_url in string value (false positive with old implementation)",
configJSON: `{"description": "Set \"remote_url\" in config to enable remote mode"}`,
expected: false,
},
{
name: "remote_url field is empty string",
configJSON: `{"remote_url": ""}`,
expected: false,
},
{
name: "remote_url field with whitespace only",
configJSON: `{"remote_url": " "}`,
expected: false,
},
}

for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

// Test the JSON parsing logic directly
var config struct {
RemoteURL string `json:"remote_url"`
}

err := json.Unmarshal([]byte(tt.configJSON), &config)
if err != nil {
t.Fatalf("failed to parse JSON: %v", err)
}

result := strings.TrimSpace(config.RemoteURL) != ""
Copy link
Contributor

@jhrozek jhrozek Dec 18, 2025

Choose a reason for hiding this comment

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

Test duplicates implementation logic instead of calling actual method

This test replicates the JSON parsing logic from isRemoteWorkload rather than calling the actual method. While this validates the parsing logic works, it doesn't provide regression protection if the method's behavior changes.

Suggested improvement:

  1. Create a mock state.Store that returns a reader with the test JSON
  2. Construct a fileStatusManager with the mock store
  3. Call manager.isRemoteWorkload(ctx, workloadName) directly
  4. Assert on the returned bool and error

This would ensure the test exercises the full code path including store interaction, reader handling, and the defer cleanup.

Note: This is a low-priority suggestion - the existing tests for GetWorkload do exercise isRemoteWorkload through integration paths, so there is some coverage.


if result != tt.expected {
t.Errorf("expected %v, got %v for JSON: %s", tt.expected, result, tt.configJSON)
}
})
}
Comment on lines +1906 to +1927
Copy link

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

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

The test reimplements the JSON parsing logic instead of testing the actual isRemoteWorkload function. This creates a disconnect where the test could pass even if the actual implementation has bugs.

Consider testing the actual isRemoteWorkload method by:

  1. Creating a fileStatusManager with mocked runConfigStore
  2. Setting up the mock to return different JSON configurations
  3. Calling manager.isRemoteWorkload(ctx, "test-workload") and verifying the result

Example pattern used elsewhere in this file (see lines 103-111):

manager, _, mockRunConfigStore := newTestFileStatusManager(t, ctrl)
mockRunConfigStore.EXPECT().Exists(gomock.Any(), "test-workload").Return(true, nil)
mockReader := io.NopCloser(strings.NewReader(tt.configJSON))
mockRunConfigStore.EXPECT().GetReader(gomock.Any(), "test-workload").Return(mockReader, nil)
result, err := manager.isRemoteWorkload(ctx, "test-workload")

Copilot uses AI. Check for mistakes.
}
Loading