-
Notifications
You must be signed in to change notification settings - Fork 156
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.
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
base: main
Are you sure you want to change the base?
Conversation
…_2941 Signed-off-by: deepika1214 <deepikav201818@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2968 +/- ##
=======================================
Coverage 56.11% 56.12%
=======================================
Files 328 328
Lines 32399 32401 +2
=======================================
+ Hits 18181 18185 +4
+ Misses 12670 12669 -1
+ Partials 1548 1547 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: deepika1214 <deepikav201818@gmail.com>
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.
Pull request overview
This PR fixes a bug in the isRemoteWorkload function where string matching on raw JSON could produce false positives. The fix replaces string matching with proper JSON parsing to accurately detect the presence and value of the remote_url field.
Key Changes
- Replaced
io.ReadAll+strings.Containswithjson.NewDecoderfor proper JSON parsing inisRemoteWorkload - Added whitespace trimming check to ensure empty or whitespace-only URLs are not considered remote
- Added comprehensive test cases covering edge cases including the false positive scenario
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/workloads/statuses/file_status.go | Refactored isRemoteWorkload to use JSON parsing instead of string matching; removed unnecessary io import |
| pkg/workloads/statuses/file_status_test.go | Added edge case tests for remote workload detection including false positive prevention |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) != "" | ||
|
|
||
| if result != tt.expected { | ||
| t.Errorf("expected %v, got %v for JSON: %s", tt.expected, result, tt.configJSON) | ||
| } | ||
| }) | ||
| } |
Copilot
AI
Dec 10, 2025
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.
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:
- Creating a
fileStatusManagerwith mockedrunConfigStore - Setting up the mock to return different JSON configurations
- 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")
Summary
Replaces string matching with proper JSON parsing in the
isRemoteWorkloadfunction to prevent false positives when detecting remote workloads.Fixes
Closes #2941
Testing
All existing tests pass. New test validates the fix prevents false positives when JSON contains
"remote_url"as part of a string value rather than as a field name.