Add activities when toggling GitOps exception settings - #44094
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44094 +/- ##
=======================================
Coverage 66.77% 66.77%
=======================================
Files 2629 2629
Lines 211234 211264 +30
Branches 9535 9502 -33
=======================================
+ Hits 141041 141075 +34
+ Misses 57365 57362 -3
+ Partials 12828 12827 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds two GitOps-exception activity types ("enabled_gitops_exception" and "disabled_gitops_exception") end-to-end. Frontend: extends ActivityType enum, makes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx`:
- Around line 961-965: The editedGitOpsException formatter produces awkward text
when activity.details or its fields are missing; update the
editedGitOpsException function (signature editedGitOpsException: (activity:
IActivity) => { ... }) to first guard that activity.details and
activity.details.exception and activity.details.enabled are present, and if
either is missing return a safe fallback (e.g., the default template or a
generic message like "updated GitOps exceptions") instead of interpolating empty
values; if both fields exist, produce the current "{enabled/disabled} the
{exception} exception for GitOps." string.
In `@server/service/appconfig.go`:
- Around line 861-883: The code emits ActivityTypeEditedGitOpsException
activities (via svc.NewActivity) while comparing
oldAppConfig.GitOpsConfig.Exceptions to appConfig.GitOpsConfig.Exceptions before
persisting changes; move the loop that builds/sends these activities to after
the call to SaveAppConfig so activities are only created if SaveAppConfig
succeeds, i.e. compute exceptionChanges using oldAppConfig and appConfig as now
but defer invoking svc.NewActivity (and constructing
ActivityTypeEditedGitOpsException) until after the SaveAppConfig call returns
without error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5bc135e0-31da-42d0-952c-92000fb784a4
📥 Commits
Reviewing files that changed from the base of the PR and between fa97579 and 95d030c2fd0acf2b28cedaf79203e82a21b980c1.
⛔ Files ignored due to path filters (1)
docs/Contributing/reference/audit-logs.mdis excluded by!**/*.md
📒 Files selected for processing (7)
frontend/interfaces/activity.tsfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsxfrontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsxserver/fleet/activities.goserver/service/appconfig.goserver/service/appconfig_test.goserver/service/integration_enterprise_test.go
| editedGitOpsException: (activity: IActivity) => { | ||
| const verb = activity.details?.enabled ? "enabled" : "disabled"; | ||
| const exception = activity.details?.exception ?? ""; | ||
| return `${verb} the ${exception} exception for GitOps.`; | ||
| }, |
There was a problem hiding this comment.
Minor: degenerate output when exception is missing.
If activity.details?.exception is absent, the rendered string is "enabled the exception for GitOps." (double space, ungrammatical). Similarly, an undefined enabled silently renders as "disabled", which could misrepresent history if the backend ever omits the flag. Consider falling back to the default template (or skipping the verb/exception entirely) when the expected fields are missing.
🛠️ Suggested guard
editedGitOpsException: (activity: IActivity) => {
- const verb = activity.details?.enabled ? "enabled" : "disabled";
- const exception = activity.details?.exception ?? "";
- return `${verb} the ${exception} exception for GitOps.`;
+ const { enabled, exception } = activity.details ?? {};
+ if (enabled === undefined || !exception) {
+ return TAGGED_TEMPLATES.defaultActivityTemplate(activity);
+ }
+ const verb = enabled ? "enabled" : "disabled";
+ return `${verb} the ${exception} exception for GitOps.`;
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| editedGitOpsException: (activity: IActivity) => { | |
| const verb = activity.details?.enabled ? "enabled" : "disabled"; | |
| const exception = activity.details?.exception ?? ""; | |
| return `${verb} the ${exception} exception for GitOps.`; | |
| }, | |
| editedGitOpsException: (activity: IActivity) => { | |
| const { enabled, exception } = activity.details ?? {}; | |
| if (enabled === undefined || !exception) { | |
| return TAGGED_TEMPLATES.defaultActivityTemplate(activity); | |
| } | |
| const verb = enabled ? "enabled" : "disabled"; | |
| return `${verb} the ${exception} exception for GitOps.`; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx`
around lines 961 - 965, The editedGitOpsException formatter produces awkward
text when activity.details or its fields are missing; update the
editedGitOpsException function (signature editedGitOpsException: (activity:
IActivity) => { ... }) to first guard that activity.details and
activity.details.exception and activity.details.enabled are present, and if
either is missing return a safe fallback (e.g., the default template or a
generic message like "updated GitOps exceptions") instead of interpolating empty
values; if both fields exist, produce the current "{enabled/disabled} the
{exception} exception for GitOps." string.
There was a problem hiding this comment.
Pull request overview
Adds a new audit-log activity for GitOps exception toggles (labels/software/secrets), wiring it end-to-end from backend activity generation through UI rendering and documentation.
Changes:
- Introduces
edited_gitops_exceptionactivity type withexception+enableddetails. - Emits an activity per exception flag change when modifying app config.
- Updates UI activity feed rendering/filter labels and adds backend/frontend tests + audit-log docs.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/integration_enterprise_test.go | Extends GitOps exceptions integration test to assert activities are generated. |
| server/service/appconfig.go | Emits edited_gitops_exception activities when exception flags change. |
| server/service/appconfig_test.go | Adds unit test coverage for exception activity emission behavior. |
| server/fleet/activities.go | Registers new ActivityTypeEditedGitOpsException activity detail type. |
| frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx | Renders new activity type in the global activity feed. |
| frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tests.tsx | Adds UI tests for enabled/disabled exception messages. |
| frontend/interfaces/activity.ts | Adds enum value + detail fields + filter label for the new activity type. |
| docs/Contributing/reference/audit-logs.md | Documents edited_gitops_exception payload shape and example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/testing_client.go (1)
687-710: Optional: parsewantDetailsonce and compare withreflect.DeepEqual.
json.Unmarshal([]byte(details), &wantDetails)is re-run for every activity even thoughdetailsis invariant across the loop, and the marshal-then-string-compare round trip is only needed because we're comparing structurally. Parsing once outside the loop and usingreflect.DeepEqualon the decodedinterface{}values is equivalent (numbers round-trip tofloat64, strings tostring, etc.), cheaper, and drops the ignored marshal errors.♻️ Proposed refactor
func (ts *withServer) lastActivityOfTypeMatches(name, details string, id uint) uint { t := ts.s.T() var listActivities listActivitiesResponse ts.DoJSON("GET", "/api/latest/fleet/activities", nil, http.StatusOK, &listActivities, "order_key", "a.id", "order_direction", "desc", "per_page", "10") require.True(t, len(listActivities.Activities) > 0) + var wantDetails interface{} + if details != "" { + require.NoError(t, json.Unmarshal([]byte(details), &wantDetails)) + } + for _, act := range listActivities.Activities { if act.Type == name { if details != "" { if act.Details == nil { continue } - // Use details as a filter: skip activities whose details don't match. - var wantDetails, gotDetails interface{} - require.NoError(t, json.Unmarshal([]byte(details), &wantDetails)) + // Use details as a filter: skip activities whose details don't match. + var gotDetails interface{} if err := json.Unmarshal([]byte(*act.Details), &gotDetails); err != nil { continue } - wantJSON, _ := json.Marshal(wantDetails) - gotJSON, _ := json.Marshal(gotDetails) - if string(wantJSON) != string(gotJSON) { + if !reflect.DeepEqual(wantDetails, gotDetails) { continue } } if id > 0 { assert.Equal(t, id, act.ID) } return act.ID } }(Requires adding
"reflect"to the imports.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/service/testing_client.go` around lines 687 - 710, Move the one-time parsing of the test `details` string out of the loop in the function that iterates over `listActivities.Activities` (the loop shown) by calling `json.Unmarshal([]byte(details), &wantDetails)` once before the for-loop (and fail the test if that unmarshal errors, e.g., with `require.NoError`), add the "reflect" import, and inside the loop unmarshal each activity's `act.Details` into `gotDetails` and compare with `reflect.DeepEqual(wantDetails, gotDetails)` instead of re-parsing `details` repeatedly and using the marshal/string comparison; keep the existing ID assertion (`assert.Equal(t, id, act.ID)`) and same continue/return control flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/service/testing_client.go`:
- Around line 687-710: Move the one-time parsing of the test `details` string
out of the loop in the function that iterates over `listActivities.Activities`
(the loop shown) by calling `json.Unmarshal([]byte(details), &wantDetails)` once
before the for-loop (and fail the test if that unmarshal errors, e.g., with
`require.NoError`), add the "reflect" import, and inside the loop unmarshal each
activity's `act.Details` into `gotDetails` and compare with
`reflect.DeepEqual(wantDetails, gotDetails)` instead of re-parsing `details`
repeatedly and using the marshal/string comparison; keep the existing ID
assertion (`assert.Equal(t, id, act.ID)`) and same continue/return control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e2c270f-8e2a-481a-b688-14b374db3cfa
📥 Commits
Reviewing files that changed from the base of the PR and between 37d9e943d7c69f5f328b0d4abb6516ac3f66e130 and fff22e5d0a396c4d2443ca22eefb16575192863f.
📒 Files selected for processing (1)
server/service/testing_client.go
fff22e5 to
b74f8a7
Compare
…ltiple activities of same type Agent-Logs-Url: https://github.com/fleetdm/fleet/sessions/3f5bd28b-9995-4261-936a-9ee404b35532 Co-authored-by: sgress454 <553428+sgress454@users.noreply.github.com>
76ae026 to
8191015
Compare
sgress454
changed the title
lukeheath
left a comment
lukeheath
left a comment
There was a problem hiding this comment.
Looks good! Reviewed the Go, as well.
Related issue: For #40171
Details
Adds audit activity when enabling or disabling GitOps exceptions.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
n/a, unreleased
Testing
For unreleased bug fixes in a release candidate, one of:
Summary by CodeRabbit