Record per-host ticket policy automation activities - #47203
Conversation
juan-fdz-hawa
changed the title
juan-fdz-hawa
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 46899-policy-status-page---implement-missing-hosts-activities--all #47203 +/- ##
===================================================================================================
Coverage 67.20% 67.20%
===================================================================================================
Files 2946 2946
Lines 226504 226571 +67
Branches 11683 11683
===================================================================================================
+ Hits 152217 152271 +54
- Misses 60541 60551 +10
- Partials 13746 13749 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
WalkthroughThis PR adds per-host activity logging for policy ticket automation outcomes in Jira and Zendesk integrations. The change introduces error response truncation utilities (bounded to 512 KiB at UTF-8 boundaries), two new activity types for failed and queued ticket automations, and a worker final-failure notification hook. Jira issue creation now captures HTTP response bodies on failure. Both Jira and Zendesk workers record activities on success (ticket created) and failure (after retries exhausted), with comprehensive test coverage throughout. 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 docstrings
🧪 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/service/externalsvc/jira.go`:
- Around line 92-97: Replace the manual io.LimitReader + string conversion for
resp.Response.Body with the truncation utility to ensure UTF-8 safety and a
truncation marker: read up to str.MaxErrorResponseBytes+1 bytes from
resp.Response.Body (still guarding OOM), close the body, then call
str.TruncateErrorResponse on the byte slice and assign the result to respBody;
update the block around resp.Response.Body handling in jira.go (the section that
currently uses io.LimitReader and b := io.ReadAll(...)) to use this
read+str.TruncateErrorResponse flow so multi-byte runes are not cut and
"[truncated]" is appended when appropriate.
🪄 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: 7d573718-ab67-4f0f-a130-3f9e0224bdc3
📒 Files selected for processing (15)
changes/38670-policy-status-pagecmd/fleet/cron.gocmd/fleet/serve.gopkg/str/str.gopkg/str/str_test.goserver/fleet/activities.goserver/fleet/activities_test.goserver/service/externalsvc/jira.goserver/service/externalsvc/jira_test.goserver/worker/jira.goserver/worker/jira_test.goserver/worker/worker.goserver/worker/worker_test.goserver/worker/zendesk.goserver/worker/zendesk_test.go
nulmete
left a comment
nulmete
left a comment
There was a problem hiding this comment.
Looks great. I have some suggestions below which I don't think are blocking, but worth addressing IMHO. Let me know what you think and I can re-review/approve.
| // ActivityTypeRanAutomationTicket is recorded when a failing-policy | ||
| // ticket automation (Jira or Zendesk) successfully creates the ticket. It is | ||
| // associated with every host the failing-policy job targeted. The Type field is | ||
| // "jira" or "zendesk". For Jira, TicketKey holds the issue key (e.g. "ENG-24"); | ||
| // for Zendesk, TicketID holds the numeric ticket ID. |
There was a problem hiding this comment.
I'm wondering if we really need this comment at all.
is recorded when a failing-policy // ticket automation (Jira or Zendesk) successfully creates the ticket
and
For Jira, TicketKey holds the issue key (e.g. "ENG-24"); // for Zendesk, TicketID holds the numeric ticket ID.
IMHO, for those 2 pieces we should be able to understand this by looking at how and where this struct is used.
The Type field is // "jira" or "zendesk"
nit: any chance that we can use a typed string / enum for this?
I'd say the same applies for the comment above ActivityTypeFailedAutomationTicket.
| func (j *Jira) OnFinalFailure(ctx context.Context, argsJSON json.RawMessage, jobErr string) error { | ||
| if j.NewActivitySvc == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
is there any case where NewActivitySvc would be nil? Reading serve.go I see that it's always passed. If this is just for tests, could tests be adjusted to have this service injected?
same for the check on L355 below.
| // OnFinalFailure records a failed_automation_ticket host activity once | ||
| // the worker has exhausted all retries for a failing-policy job. Vulnerability | ||
| // jobs are ignored as they are not host- or policy-scoped. |
There was a problem hiding this comment.
Something similar to what I commented on the server/fleet/activities.go file. Perhaps we can drop this comment (if I see where this is called I can understand at first glance that this function runs when job.Retries reached maxRetries).
Re:
Vulnerability jobs are ignored as they are not host- or policy-scoped.
Are those Vulnerability jobs ignored because of the args.FailingPolicy == nil check below? Maybe I'd put the // Vulnerability jobs are ignored as they are not host- or policy-scoped. comment just above that line. (I had to look at jiraArgs and then I understood why.)
| t.Run("nil activity service is a no-op", func(t *testing.T) { | ||
| j := &Jira{ | ||
| FleetURL: "https://fleetdm.com", | ||
| Datastore: ds, | ||
| Log: slog.New(slog.DiscardHandler), | ||
| NewClientFunc: func(opts *externalsvc.JiraOptions) (JiraClient, error) { | ||
| return client, nil | ||
| }, | ||
| } | ||
|
|
||
| args := json.RawMessage(`{"failing_policy":{"policy_id":5,"policy_name":"p5","hosts":[]}}`) | ||
| require.NoError(t, j.Run(license.NewContext(context.Background(), &fleet.LicenseInfo{Tier: fleet.TierFree}), args)) |
There was a problem hiding this comment.
I don't think we'll need this test since we always pass the activity service when serving, correct?
| return t.run(ctx, argsJSON) | ||
| } | ||
|
|
||
| // testJobNotifier is a testJob that also implements FinalFailureNotifier. |
There was a problem hiding this comment.
nit: IMO we can drop the comment 😄
| t.Run("nil activity service is a no-op", func(t *testing.T) { | ||
| z := &Zendesk{ | ||
| FleetURL: "https://fleetdm.com", | ||
| Datastore: ds, | ||
| Log: slog.New(slog.DiscardHandler), | ||
| NewClientFunc: func(opts *externalsvc.ZendeskOptions) (ZendeskClient, error) { | ||
| return client, nil | ||
| }, | ||
| } | ||
|
|
||
| args := json.RawMessage(`{"failing_policy":{"policy_id":6,"policy_name":"p6","hosts":[]}}`) | ||
| require.NoError(t, z.Run(license.NewContext(context.Background(), &fleet.LicenseInfo{Tier: fleet.TierFree}), args)) | ||
| }) |
There was a problem hiding this comment.
same comment that I made in the jira_test.go file
| // OnFinalFailure records a failed_automation_ticket host activity once | ||
| // the worker has exhausted all retries for a failing-policy job. Vulnerability | ||
| // jobs are ignored as they are not host- or policy-scoped. | ||
| func (z *Zendesk) OnFinalFailure(ctx context.Context, argsJSON json.RawMessage, jobErr string) error { | ||
| if z.NewActivitySvc == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
same comment that I made in the jira.go file (the comment and the nil checks)
Thanks for reviewing this - While those comments might seem a bit redundant to us, they provide helpful context for AI tools without requiring a deep dive into the implementation details. I’m inclined to keep them unless you see a specific downside? |
No big downsides IMO, maybe:
|
| // be activated, because the current activity was never truly started — | ||
| // activating the next one would break the intended sequential ordering. | ||
| // | ||
| // See commit 159194acc9d92843bb2de933309f159c84a501aa for the fix. |
There was a problem hiding this comment.
Oh this was removed by 🤖 - let me add it back
bf815c1
into
46899-policy-status-page---implement-missing-hosts-activities--all
3 participants
Related issue: Resolves #46899
Added per-host activity log entries when policy ticket automations (Jira or Zendesk) fail after all retries are exhausted or succeed (ticket created).
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.
Testing
Summary by CodeRabbit
New Features
Improvements