Adds activities for certificate templates - #36903
Conversation
|
|
||
| if _, err := ds.writer(ctx).ExecContext(ctx, stmt, args...); err != nil { | ||
| return ctxerr.Wrap(ctx, err, "upserting certificate_templates") | ||
| if insertOnDuplicateDidInsertOrUpdate(result) { |
There was a problem hiding this comment.
With CLIENT_FOUND_ROWS enabled, RowsAffected returns 1 even when setting a row to its current values. Use insertOnDuplicateDidInsertOrUpdatewhich checks LastInsertId to determine if a real insert/update occurred.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #36903 +/- ##
==========================================
+ Coverage 65.95% 65.98% +0.02%
==========================================
Files 2336 2336
Lines 185738 185673 -65
Branches 7754 7835 +81
==========================================
+ Hits 122509 122513 +4
+ Misses 52036 51963 -73
- Partials 11193 11197 +4
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:
|
| "team_id": 123, | ||
| "team_name": "Workstations" | ||
| }` | ||
| } |
There was a problem hiding this comment.
There is only 1 (edit) in the spec. However, we have the opportunity to have more granular activities, created/edited and deleted. I think it would probably be worth a split.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe PR extends certificate template datastore methods to return modification metadata—teams map for upserts, deletion indicator for deletes—enabling the service layer to log activities when certificate templates are edited via GitOps. New activity type, frontend UI support, and comprehensive test coverage are added. Changes
Sequence DiagramsequenceDiagram
participant Client as Client / GitOps
participant Service as Service Layer
participant DS as Datastore
participant ActivityMgr as Activity Manager
participant Team as Team Lookup
Client->>Service: ApplyCertificateTemplateSpecs(templates)
Service->>DS: BatchUpsertCertificateTemplates(templates)
DS-->>Service: (modifiedTeamsMap, error)
alt teamsModified not empty
loop for each modified team
Service->>Team: Fetch team details
Team-->>Service: team data
Service->>ActivityMgr: NewActivity(EditedAndroidCertificate)
ActivityMgr-->>Service: activity created
end
end
Service-->>Client: success
Client->>Service: DeleteCertificateTemplates(ids)
Service->>DS: BatchDeleteCertificateTemplates(ids)
DS-->>Service: (rowsDeleted: bool, error)
alt rowsDeleted = true
Service->>Team: Fetch team details
Team-->>Service: team data
Service->>ActivityMgr: NewActivity(EditedAndroidCertificate)
ActivityMgr-->>Service: activity created
end
Service-->>Client: success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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: 0
🧹 Nitpick comments (6)
server/fleet/activities.go (1)
3142-3159: Clarify docs to cover all “edit” cases (add/update/remove) for cert templatesThe activity name (
edited_android_certificate) and service behavior cover any GitOps edit (upsert or delete), but the doc text only calls out “adds or removes.” To avoid confusion for API/webhook consumers, consider phrasing that explicitly includes updates, e.g.:-func (a ActivityTypeEditedAndroidCertificate) Documentation() (activity, details, detailsExample string) { - return `Generated when a user adds or removes Android certificate templates of a team (or no team) via the fleetctl CLI.`, +func (a ActivityTypeEditedAndroidCertificate) Documentation() (activity, details, detailsExample string) { + return `Generated when a user edits Android certificate templates (adds, updates, or removes) for a team (or no team) via the fleetctl CLI.`,The rest of the details block and example JSON look consistent with other team‑scoped “edited_*” activities.
server/service/integration_core_test.go (4)
8004-8033: Activity count assertions around certificate spec apply look correct but are slightly brittleThe pattern of capturing
activitiesBeforeInsertand then assertinglen(activitiesAfterInsert) == len(activitiesBeforeInsert)+1for a single team-scoped apply is sound and matches the “one activity per team per apply” contract. Likewise, usinglastActivityMatchesto assert the most recent activity isActivityTypeEditedAndroidCertificatewith the expectedteam_id/team_namepayload is a good end‑to‑end guard.The only brittleness here is the strict string comparison of the JSON details; if the activity detail struct ever changes field order, these tests will fail despite equivalent semantics. If you ever touch
ActivityTypeEditedAndroidCertificate, consider either:
- making
lastActivityMatchesparse JSON and compare by fields, or- using
assert.JSONEqon the details string instead of a raw equality.For the current PR, the logic is correct and aligned with the new behavior.
8041-8062: Idempotency check for certificate spec re‑apply is good but depends on helper semanticsCapturing
lastActivityID := s.lastActivityMatches("", "", 0)before the secondPOST /spec/certificatesand then asserting thatcurrentActivityIDis unchanged is an effective way to confirm that a no‑op re‑apply doesn’t emit a new activity.This assumes
lastActivityMatches("", "", 0)reliably returns the latest activity with no filtering, which is consistent with other tests in this suite. If that helper ever changes to enforce non‑empty type/details, these calls will become confusing; a small future improvement would be to introduce a dedicated helper (e.g.,lastActivityID()) instead of passing empty strings.Behavior-wise, this is correct and gives good coverage of the idempotent path.
8131-8141: Delete-spec test correctly scopes by team and reuses the edited-certificate activityUsing the batch delete with both
idsandteam_idthen asserting anActivityTypeEditedAndroidCertificatewith matchingteam_id/team_nameis a good mirror of the apply coverage and verifies that deletes are logged through the same activity type.Given you already assert
require.Len(t, listCertifcatesResp.Certificates, 2)earlier, indexing[0]and[1]here is safe. As above, the raw JSON detail comparison could be fragile if struct field order changes, but that’s consistent with other tests and not blocking.
8147-8182: No‑team (“global”) certificate activity coverage is solidThe pattern of:
- capturing
activitiesBeforeNoTeam,- applying three “no team” templates in a single request, and
- asserting
len(activitiesAfterNoTeam) == len(activitiesBeforeNoTeam)+1plus a finalEditedAndroidCertificateactivity with{"team_id": null, "team_name": null}correctly verifies the “one activity per apply when team_id is nil” behavior.
Same minor note as above: relying on an exact JSON string for the details (including field order and explicit
nulls) is somewhat brittle, but acceptable given current struct layout. If this test ever becomes flaky after activity struct changes, switching to JSON field comparison would make it more resilient.Functionally, this test is well‑targeted and matches the intended semantics.
server/datastore/mysql/certificate_templates.go (1)
205-206: Consider handling the error fromRowsAffected().The error from
result.RowsAffected()is being ignored with_. While this error is typically nil for MySQL drivers, defensive coding would handle it:- rowsAffected, _ := result.RowsAffected() - return rowsAffected > 0, nil + rowsAffected, err := result.RowsAffected() + if err != nil { + return false, ctxerr.Wrap(ctx, err, "getting rows affected for batch delete certificate_templates") + } + return rowsAffected > 0, nil
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmd/fleetctl/fleetctl/gitops_test.go(5 hunks)frontend/interfaces/activity.ts(2 hunks)frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx(2 hunks)server/datastore/mysql/certificate_templates.go(2 hunks)server/datastore/mysql/certificate_templates_test.go(7 hunks)server/fleet/activities.go(2 hunks)server/fleet/datastore.go(1 hunks)server/mock/datastore_mock.go(2 hunks)server/service/certificate_templates_test.go(3 hunks)server/service/certificates.go(3 hunks)server/service/integration_core_test.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/fleet/datastore.goserver/datastore/mysql/certificate_templates.goserver/fleet/activities.goserver/service/certificate_templates_test.goserver/service/certificates.goserver/mock/datastore_mock.goserver/service/integration_core_test.gocmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/certificate_templates_test.go
🧠 Learnings (3)
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet repository tests (server/datastore/mysql/labels_test.go and similar), using testing.T.Context() is valid because the project targets a recent Go version where testing.T.Context() exists. Do not suggest replacing t.Context() with context.Background() in this codebase.
Applied to files:
server/service/certificate_templates_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: Fleet repo targets Go 1.24.5 (root go.mod), which supports testing.T.Context(). Do not flag usage of t.Context() or suggest replacing it with context.Background() in tests (e.g., server/datastore/mysql/labels_test.go Line 2031 and similar).
Applied to files:
server/service/certificate_templates_test.gocmd/fleetctl/fleetctl/gitops_test.goserver/datastore/mysql/certificate_templates_test.go
📚 Learning: 2025-08-08T07:40:05.301Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31726
File: server/datastore/mysql/labels_test.go:2031-2031
Timestamp: 2025-08-08T07:40:05.301Z
Learning: In fleetdm/fleet, tests may validly use testing.T.Context() when the module/toolchain targets Go 1.24+. Do not flag t.Context() usage in this codebase if go.mod/toolchain indicates Go >= 1.24.
Applied to files:
server/service/certificate_templates_test.gocmd/fleetctl/fleetctl/gitops_test.go
🧬 Code graph analysis (5)
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx (2)
frontend/interfaces/activity.ts (1)
IActivity(177-188)frontend/utilities/permissions/permissions.ts (1)
isPremiumTier(12-14)
server/fleet/activities.go (1)
server/service/certificate_templates_test.go (1)
TeamID(29-29)
server/service/certificate_templates_test.go (4)
server/mock/datastore_mock.go (4)
AppConfigFunc(362-362)TeamLiteFunc(424-424)NewActivityFunc(550-550)BatchUpsertCertificateTemplatesFunc(1642-1642)server/fleet/teams.go (1)
TeamLite(98-109)server/fleet/activities.go (1)
ActivityDetails(256-261)server/fleet/certificate_templates.go (1)
CertificateTemplate(10-15)
server/service/certificates.go (5)
server/fleet/teams.go (1)
TeamLite(98-109)server/contexts/ctxerr/ctxerr.go (1)
Wrap(199-202)orbit/pkg/keystore/keystore_darwin.go (1)
Name(29-31)server/fleet/activities.go (1)
ActivityTypeEditedAndroidCertificate(3142-3145)server/service/certificate_templates_test.go (1)
TeamID(29-29)
server/datastore/mysql/certificate_templates_test.go (1)
server/fleet/certificate_templates.go (1)
CertificateTemplate(10-15)
🔇 Additional comments (24)
server/fleet/datastore.go (1)
2519-2523: Interface changes for certificate template batch ops look goodThe updated return types and comments for
BatchUpsertCertificateTemplatesandBatchDeleteCertificateTemplatesare clear and consistent with a Go-style API surface.frontend/interfaces/activity.ts (1)
80-80: New Android certificate activity type is wired correctly
ActivityType.EditedAndroidCertificateand its display name mapping are added consistently with existing GitOps-related activity types.Also applies to: 413-414
frontend/pages/DashboardPage/cards/ActivityFeed/GlobalActivityItem/GlobalActivityItem.tsx (1)
567-580: EditedAndroidCertificate activity rendering is consistent and correctThe new
editedAndroidCertificatetemplate and itsgetDetailswitch case mirror the existing Android profile GitOps patterns, correctly handling team scoping and premium tier behavior.Also applies to: 1729-1731
server/fleet/activities.go (1)
187-191: LGTM: Activity type registered for docs and auditingAdding
ActivityTypeEditedAndroidCertificate{}toActivityDetailsListkeeps Android certificate edits documented and exposed consistently alongside the other Android profile activities. No issues here.server/service/certificate_templates_test.go (2)
78-91: LGTM! Mock functions correctly support the activity logging dependencies.The added mocks for
AppConfigFunc,TeamLiteFunc, andNewActivityFuncproperly provide the minimal implementations needed for the service layer to log activities when certificate templates are modified via GitOps.
138-145: LGTM! BatchUpsertCertificateTemplatesFunc mock correctly returns the new signature.The mock correctly constructs the
createdMapby usingcert.TeamIDas the key, which aligns with the updated datastore method signature that returns(map[uint]bool, error).server/datastore/mysql/certificate_templates_test.go (5)
632-635: LGTM! Test correctly captures and validates empty teamsModified map for empty input.The test verifies that calling
BatchUpsertCertificateTemplateswith an empty slice returns an emptyteamsModifiedmap.
672-681: LGTM! Test correctly validates teamsModified map contains the team ID after creating certificates.The test asserts that after upserting two certificates for the same team, the
teamsModifiedmap has length 1 and contains the team ID.
770-773: LGTM! Test correctly validates generateActivity is false for empty input.The test verifies that calling
BatchDeleteCertificateTemplateswith an empty slice returnsfalseforgenerateActivity.
837-841: LGTM! Test correctly validates generateActivity is true after deleting certificates.The test asserts that after deleting existing certificates,
generateActivityistrue, indicating deletions occurred.
721-734: The test correctly validates thatteamsModifiedis empty when upserting an unchanged certificate.The
ON DUPLICATE KEY UPDATEclause (lines 161-163 in certificate_templates.go) updates onlynameandteam_id. Since both remain unchanged in this test scenario, the upsert operation correctly reports no modification. The assertion thatteamsModifiedhas length 0 is valid.server/datastore/mysql/certificate_templates.go (1)
149-179: LGTM! BatchUpsertCertificateTemplates correctly implements per-team modification tracking.The implementation correctly:
- Returns
(nil, nil)for empty input.- Uses
insertOnDuplicateDidInsertOrUpdate(result)to determine if a real insert/update occurred, addressing the previous review feedback.- Populates the
teamsModifiedmap with team IDs that had modifications.cmd/fleetctl/fleetctl/gitops_test.go (7)
3808-3816: LGTM! Mock correctly implements the updated BatchUpsertCertificateTemplatesFunc signature.The mock properly constructs the
createdMapusingcert.TeamIDas the key and returns it alongsidenilerror.
3822-3824: LGTM! Mock correctly returns(false, nil)for BatchDeleteCertificateTemplatesFunc.This mock returns
falseindicating no deletion activity occurred, which is appropriate for test cases where deletion is not the focus.
3897-3905: LGTM! Mock implementation consistent with other test files.The
BatchUpsertCertificateTemplatesFuncmock follows the same pattern as other test files.
3950-3953: LGTM! Mock correctly returns(true, nil)to indicate deletion occurred.This mock returns
trueforgenerateActivity, indicating deletions were performed.
4144-4147: LGTM! Consistent mock implementation for delete operation.
4149-4157: LGTM! Consistent mock implementation for upsert operation.
4272-4275: LGTM! Consistent mock implementation for delete all operation.server/mock/datastore_mock.go (2)
1642-1644: Updated batch certificate template function signatures are consistentThe new return types for
BatchUpsertCertificateTemplatesFuncandBatchDeleteCertificateTemplatesFunccorrectly expose additional metadata while remaining consistent with how other function aliases are defined in this mock file. No issues here.
9791-9803: DataStore methods correctly propagate new batch certificate return valuesBoth
BatchUpsertCertificateTemplatesandBatchDeleteCertificateTemplatesfollow the established lock/flag/delegate pattern and correctly return the newmap[uint]bool/boolvalues from their respectiveFuncfields. This keeps the mock aligned with the datastore interface and supports the new activity-logging behavior.server/service/certificates.go (3)
8-8: LGTM!The
authzimport is correctly added to supportauthz.UserFromContext(ctx)for activity logging.
414-444: LGTM!The delete activity logging is well-structured:
- Early return when no rows deleted avoids spurious activities
- Proper nil handling for no-team case (teamID == 0)
- Error wrapping with descriptive messages
The team lookup and activity logging pattern is duplicated with the upsert flow. Consider extracting a helper if this pattern is used elsewhere, but acceptable as-is given the small scope.
366-385: The review comment is not applicable—teamsModifiedonly contains teams that were actually modified.The
BatchUpsertCertificateTemplatesimplementation (server/datastore/mysql/certificate_templates.go:166-178) only adds a team to the map wheninsertOnDuplicateDidInsertOrUpdate(result)returns true. Teams with no changes are not included in the map at all. The boolean value is alwaystruefor entries present in the map, making the current loop iteration correct.The return type
map[uint]boolcould be simplified to[]uintormap[uint]struct{}for clarity, but the code has no functional issue.
| } | ||
|
|
||
| func (ds *Datastore) BatchUpsertCertificateTemplates(ctx context.Context, certificateTemplates []*fleet.CertificateTemplate) error { | ||
| func (ds *Datastore) BatchUpsertCertificateTemplates(ctx context.Context, certificateTemplates []*fleet.CertificateTemplate) (map[uint]bool, error) { |
There was a problem hiding this comment.
Why not simplify to return ([] uint, error) or at least a (map[uint]struct{}, error)? We don't actually need the bool for anything right?
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Looks good.
Please fix the failing job: Check automated documentation is up-to-date / check-doc-gen (pull_request)
ksykulev
changed the title
getvictor
left a comment
getvictor
left a comment
There was a problem hiding this comment.
Sorry, I forgot about re-reviewing this one. I was focusing on other Android work.
Also, I recommend clicking the re-review icon so that it is clear this PR is ready for re-review.
|
@getvictor There was a merge conflict with #36978 |
|
Failures are unrelated. |
3 participants
Related issue: Resolves #36701
Checklist for submitter
If some of the following don't apply, delete the relevant line.
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.