-
Notifications
You must be signed in to change notification settings - Fork 781
[Activity changes] Add applied_spec_software activity #24913
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
[Activity changes] Add applied_spec_software activity #24913
Conversation
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.
Changes lgtm! Should this PR be to a release docs branch or was this already implemented?
|
@rachaelshaw Oh right! Let me see about moving this to a branch. Sorry! |
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.
Hey this PR should not be merged. It's pulling in all changes. Nvm just saw the above comment.
|
@eugkuo converting this PR to a draft; changing the base branch always seems to result in massive diffs (183 files changed 🫠) and auto-requests reviews from all the codeowners. I'm not sure how to fix it other than making a new PR. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## docs-v4.63.0 #24913 +/- ##
================================================
+ Coverage 63.56% 63.59% +0.02%
================================================
Files 1603 1603
Lines 151958 152006 +48
Branches 3900 3871 -29
================================================
+ Hits 96590 96661 +71
+ Misses 47667 47650 -17
+ Partials 7701 7695 -6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for doing this. Does that mean we should update the process here? |
| } | ||
| ``` | ||
|
|
||
| ## applied_spec_software |
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.
Dev note
Activity is generated when GitOps runs and there is at least one software (package, App Store app, or Fleet-maintained app) is specified in the YAML's software: https://fleetdm.com/docs/configuration/yaml-files#software
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.
cc @eugkuo
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.
I want to say this is a reversal from the (potentially inconsistently applied) pattern of "GitOps applies don't show up in the activity feed"? Seems like if we want them to show up we'd want the existing app add/edit/delete events for consistency with what manual actions add to the feed, but maybe I'm missing something?
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.
Why are adding this? There's a missing gap in activities for GitOps right now: software isn't mentioned. I think let's stick with the current pattern now to keep changes small.
we'd want the existing app add/edit/delete
Eventually we can come back to more specific language and addressing the problem of only showing activities if something changed.
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.
As we're digging back into this post-user-story-review, heads-up that the software gitops endpoints (packages and VPP apps) are always called 1x each per team, at least with Fleet Premium, and currently e.g. the script batch endpoint adds an activity whenever it's called, even if no scripts are supplied (because the bulk action may have deleted all scripts).
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.
Got it! Thanks for checking.
If I'm understanding correctly, it sounds like this new applied_spec_activity will be consistent with the other applied_spec activities. I think that's what we want 👍
Co-authored-by: Ian Littman <iansltx@gmail.com>
| { | ||
| "team_id": 123, | ||
| "team_name": "Workstations", | ||
| "software": { |
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.
@eugkuo @iansltx I just pushed some tweaks.
I think it would be nice if this matched the structure of software key in the YAML: https://fleetdm.com/docs/configuration/yaml-files#software
What do y'all think?
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.
Adding VPP into the mix would significantly affect level of effort
@iansltx by how many points you think? Might be worth it to hit VPP while we're doing this. So we don't forget to do it and never come back to it.
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.
Minimum +3, since in order to avoid broadcasting an activity when no software was provided but also broadcast an activity when software was deleted, we would need to merge VPP batch and software installer batch endpoints so we have a clear picture of before/after.
Additionally, right now fleetctl applies software packages and then VPP apps and without further digging I can't say whether that order of operations is significant. So either that order doesn't matter or it's an unknown-sized can of worms. Thinking more likely the latter since we're talking about GitOps code here.
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.
Re: the software key, if I'm consuming this JSON I'd rather iterate over one set of items and filter/null-check slug if I care about FMA, vs. iterating over two nearly identical keys (I had thought about splitting FMA out).
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.
@iansltx I think let's keep VPP in this pass. So we can get it all the way done while we're at it.
Re: the software key, if I'm consuming this JSON
I'll defer to you and @rachaelshaw (API design DRI) here. Please feel free to update the PR with your proposed changes.
|
@rachaelshaw Okee. I converted this to draft and I believe I just manually requested your review. LMK if I messed anything up. |
|
Closing this PR because we decided to de-prioritize the user story for now. |

Activity changes for the following user story: