Skip to content
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

fix issue with resetting abm token teams #26259

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ghernandez345
Copy link
Contributor

@ghernandez345 ghernandez345 commented Feb 11, 2025

For #24040

Add gitops option for the request to modify the app config.

There was an issue with the abm token teams getting reset to default anytime the PATCH /fleet/config endpoint was called. @jahzielv and I discussed various options on how to solve this and agreed that the approach taken in this PR was the quickest but not the best. Ideally, we'd like the gitops client to send back the data to the endpoint that its going to update. This will allow the PATCH /fleet/config endpoint to work like a standard PATCH request and only update the options provided instead of updating the app config differently depending on the client calling the endpoint.

  • Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
    See Changes files for more information.
  • Added/updated automated tests
  • Manual QA for all new/changed functionality

Copy link

codecov bot commented Feb 13, 2025

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.88%. Comparing base (01c9537) to head (ac2f284).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
server/service/appconfig.go 72.72% 2 Missing and 1 partial ⚠️
cmd/fleetctl/gitops.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #26259      +/-   ##
==========================================
+ Coverage   63.84%   63.88%   +0.03%     
==========================================
  Files        1632     1632              
  Lines      157936   157968      +32     
  Branches     4038     4038              
==========================================
+ Hits       100836   100912      +76     
+ Misses      49198    49155      -43     
+ Partials     7902     7901       -1     
Flag Coverage Δ
backend 64.72% <73.33%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ghernandez345 ghernandez345 marked this pull request as ready for review February 13, 2025 14:53
@ghernandez345 ghernandez345 requested a review from a team as a code owner February 13, 2025 14:53
@ghernandez345 ghernandez345 changed the title add gitops option when modifying the app config fix issue with reseting abm token teams Feb 13, 2025
@ghernandez345 ghernandez345 changed the title fix issue with reseting abm token teams fix issue with resetting abm token teams Feb 13, 2025
@getvictor
Copy link
Member

I'm sorry, I still don't understand the issue. In the code, it looks like you're checking newAppConfig.MDM.AppleBusinessManager.Set and newAppConfig.MDM.AppleBusinessManager.Value, which should be sufficient for PATCH.

It seems like you should just check newAppConfig.MDM.AppleBusinessManager.Set and do the following:

  1. No one sets anything -- leave as is.
{
  "mdm": { /* empty */ }
}
  1. Everything is cleared
{
  "mdm": {
    "apple_business_manager": null
  }
}

or

{
  "mdm": {
    "apple_business_manager": []
  }
}

I don't see the need for the new GitOps query option.

fleetctl gitops must always set something (null or array) for mdm.apple_business_manager

So, gitops should look at existing teams and set this array to match existing teams. Then, after teams have been processed (if new teams have been added), gitops can come back and patch this endpoint with the added teams. (I'm just speculating how this could work since I don't know the exact interactions.)

@getvictor getvictor self-requested a review February 13, 2025 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants