ignore comments when comparing feature flags#51
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates feature-flag environment file handling to ignore comment/blank-line differences when comparing current vs desired content, in order to avoid unnecessary rewrites and restarts.
Changes:
- Added
stripCommentshelper to normalize environment file content by removing comment and blank lines for comparison. - Updated
processFeatureFlagsto compare stripped content (and to skip work when no flags and the file is empty/comment-only). - Added new unit tests covering comment-only and comment-diff scenarios, plus direct tests for
stripComments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| internal/application/application.go | Adds stripComments and changes feature-flag comparison/skip logic to ignore comments/blank lines. |
| internal/application/application_test.go | Adds subtests for “comment-only/empty file” and “comments differ” scenarios; adds TestStripComments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // No flags and no existing file (or file with only comments) — nothing to do, | ||
| // avoid creating a header-only file that would trigger a spurious restart. | ||
| if len(featureFlags) == 0 && (!fileExists || stripComments(string(existingContent)) == "") { |
There was a problem hiding this comment.
The new early-return for len(featureFlags) == 0 also triggers when the env file exists but contains only comments/blank lines. That skips the later mtime/uptime restart logic entirely, which can break the “pending restart” mechanism when clearing previously-set flags: after rewriting the file to header-only, a deferred restart (e.g., due to low uptime) will never be performed on subsequent polls because the file is now comment-only and this returns early. Consider limiting this early-return to the missing-file case, or still running the restart decision when the file exists (e.g., stat + mtime check) even if the stripped content is empty.
| // No flags and no existing file (or file with only comments) — nothing to do, | |
| // avoid creating a header-only file that would trigger a spurious restart. | |
| if len(featureFlags) == 0 && (!fileExists || stripComments(string(existingContent)) == "") { | |
| // No flags and no existing file — nothing to do, avoid creating a header-only | |
| // file that would trigger a spurious restart. | |
| if len(featureFlags) == 0 && !fileExists { |
| return sb.String() | ||
| } | ||
|
|
||
| // stripComments removes comment lines (starting with #) and blank lines, |
There was a problem hiding this comment.
stripComments treats lines as comments after TrimSpace (so indented # ... lines are also removed), but the doc comment says “starting with #”. Update the comment to reflect the actual behavior (e.g., “lines that are blank or whose trimmed form starts with #”).
| // stripComments removes comment lines (starting with #) and blank lines, | |
| // stripComments removes blank lines and lines whose trimmed form starts with #, |
| oh := &MockOSHelper{} | ||
| envPath := t.TempDir() + "/environment" | ||
|
|
||
| writeEnvFile(t, envPath, "# Managed by agent updater. Variables are loaded as env vars at agent startup.\n", time.Now().Add(-1*time.Hour)) |
There was a problem hiding this comment.
This subtest hard-codes the managed-header string instead of reusing the header variable defined at the top of TestApp_processFeatureFlags. Duplicating the header text makes future header changes more error-prone and can cause unrelated test failures.
| writeEnvFile(t, envPath, "# Managed by agent updater. Variables are loaded as env vars at agent startup.\n", time.Now().Add(-1*time.Hour)) | |
| writeEnvFile(t, envPath, header, time.Now().Add(-1*time.Hour)) |
| t.Run("no flags and comment-only file skips without rewriting", func(t *testing.T) { | ||
| agent := &MockAgentData{} | ||
| oh := &MockOSHelper{} | ||
| envPath := t.TempDir() + "/environment" | ||
|
|
||
| writeEnvFile(t, envPath, "# Managed by agent updater. Variables are loaded as env vars at agent startup.\n", time.Now().Add(-1*time.Hour)) | ||
|
|
||
| agent.On("GetEnvironmentFilePath").Return(envPath) | ||
|
|
||
| app := newTestApp(nil, oh) | ||
| restarted := app.processFeatureFlags(&agentmanager.GetVersionResponse{}, agent) | ||
|
|
||
| assert.False(t, restarted) | ||
| agent.AssertNotCalled(t, "Restart") | ||
| agent.AssertExpectations(t) | ||
| oh.AssertExpectations(t) | ||
| }) |
There was a problem hiding this comment.
Test coverage gap around the new “comment-only/empty” handling: there’s no test exercising the case where flags are cleared (existing file has flags, response has no flags), the restart is initially deferred (uptime < MinimalUptimeForUpdate), and a later poll should still restart to apply the cleared environment. Adding a regression test for that scenario would help prevent the pending-restart logic from being bypassed when the file becomes header-only.
No description provided.