Skip to content

Add new configuration option to set default team for Apple Business Manager#9062

Merged
lukeheath merged 7 commits into
mainfrom
issue-8733-apple-business-manager-set-default-team
Jan 3, 2023
Merged

Add new configuration option to set default team for Apple Business Manager#9062
lukeheath merged 7 commits into
mainfrom
issue-8733-apple-business-manager-set-default-team

Conversation

@fx5
Copy link
Copy Markdown
Contributor

@fx5 fx5 commented Dec 19, 2022

For #8733

  • Changes file added for user-visible changes in changes/ or orbit/changes/.
    See Changes files for more information.

  • Documented any API changes (docs/Using-Fleet/REST-API.md or docs/Contributing/API-for-contributors.md)

  • Manual QA for all new/changed functionality

@fx5 fx5 force-pushed the issue-8733-apple-business-manager-set-default-team branch from 6120d1b to c1f12fd Compare December 20, 2022 16:40
@fx5 fx5 temporarily deployed to Docker Hub December 20, 2022 16:40 — with GitHub Actions Inactive
@fx5 fx5 temporarily deployed to Docker Hub December 20, 2022 16:42 — with GitHub Actions Inactive
@fx5 fx5 temporarily deployed to Docker Hub December 20, 2022 16:50 — with GitHub Actions Inactive
@fx5 fx5 marked this pull request as ready for review December 20, 2022 16:50
Comment thread cmd/fleetctl/get.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, sorry there's been a misunderstanding here - the default team should be provided as part of the API call that is done by client.GetAppleBM() above (line 1119), so there wouldn't be any change needed here. The default team is part of the response payload returned by GET /api/v1/fleet/mdm/apple_bm.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread server/fleet/app.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave a comment in AppConfig.Clone to indicate that this settings sub-section does not require any cloning (it helps see that this was not forgotten, but nothing specific needed to be done).

Comment thread server/service/appconfig.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would trigger the team lookup and validation every time the app config is modified, if there is a default team. It would be best to do it only if the request actually changes the value. You can see some examples in this function that compares the original value vs the new one, it can be used to decide if validation should be triggered. And I think the license check could be done before validating too, so that if we end up removing the default team due to the license, we don't validate the team name for nothing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have some tests too around that - call with free license + a team ensuring that it doesn't set one, and premium and invalid team/valid team. Probably at the integration level would make sense (there's TestAppConfig in integration_core_test.go for the free license, and TestCustomTransparencyURL in integration_enterprise_test.go can serve as inspiration for premium license tests).

Copy link
Copy Markdown
Contributor

@chris-mcgillicuddy chris-mcgillicuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions for your review, @fx5.

Comment thread docs/Using-Fleet/configuration-files/README.md Outdated
Comment thread docs/Using-Fleet/configuration-files/README.md Outdated
Comment thread docs/Using-Fleet/REST-API.md Outdated
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 15:43 — with GitHub Actions Inactive
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 15:43 — with GitHub Actions Inactive
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 15:44 — with GitHub Actions Inactive
@fx5 fx5 requested a review from a team December 23, 2022 15:47
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 15:47 — with GitHub Actions Inactive
@fx5 fx5 marked this pull request as draft December 23, 2022 15:50
@fx5 fx5 force-pushed the issue-8733-apple-business-manager-set-default-team branch from 2e7ffb3 to 9773595 Compare December 23, 2022 15:56
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 15:56 — with GitHub Actions Inactive
@fx5 fx5 force-pushed the issue-8733-apple-business-manager-set-default-team branch from 9773595 to 665f2a2 Compare December 23, 2022 16:02
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 16:02 — with GitHub Actions Inactive
@fx5 fx5 marked this pull request as ready for review December 23, 2022 16:04
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 23, 2022

Codecov Report

Base: 60.25% // Head: 59.80% // Decreases project coverage by -0.44% ⚠️

Coverage data is based on head (995c28b) compared to base (605ae86).
Patch coverage: 43.88% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9062      +/-   ##
==========================================
- Coverage   60.25%   59.80%   -0.45%     
==========================================
  Files         473      473              
  Lines       46063    46655     +592     
==========================================
+ Hits        27754    27902     +148     
- Misses      15705    16143     +438     
- Partials     2604     2610       +6     
Impacted Files Coverage Δ
orbit/pkg/update/flag_runner.go 22.54% <0.00%> (-24.07%) ⬇️
orbit/pkg/update/runner.go 35.13% <0.00%> (-3.11%) ⬇️
orbit/pkg/update/update.go 28.08% <0.00%> (-0.72%) ⬇️
server/fleet/activities.go 0.00% <0.00%> (ø)
server/fleet/agent_options.go 72.22% <ø> (ø)
server/fleet/app.go 0.00% <0.00%> (ø)
server/service/orbit.go 4.20% <0.00%> (ø)
server/service/orbit_client.go 0.00% <0.00%> (ø)
ee/fleetctl/updates.go 53.60% <44.44%> (-0.25%) ⬇️
server/service/users.go 67.87% <56.00%> (-0.60%) ⬇️
... and 24 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 17:27 — with GitHub Actions Inactive
Frank Sievertsen and others added 3 commits December 23, 2022 18:29
Co-authored-by: Chris McGillicuddy <108031970+chris-mcgillicuddy@users.noreply.github.com>
Co-authored-by: Chris McGillicuddy <108031970+chris-mcgillicuddy@users.noreply.github.com>
@fx5 fx5 force-pushed the issue-8733-apple-business-manager-set-default-team branch from f4de47b to 995c28b Compare December 23, 2022 17:30
@fx5 fx5 temporarily deployed to Docker Hub December 23, 2022 17:30 — with GitHub Actions Inactive
@roperzh
Copy link
Copy Markdown
Contributor

roperzh commented Dec 29, 2022

A heads-up that I have a PR based off this branch to use the setting introduced here in order to make the team assignment when a device is enrolled: #9135

@lukeheath lukeheath merged commit 91c90b4 into main Jan 3, 2023
@lukeheath lukeheath deleted the issue-8733-apple-business-manager-set-default-team branch January 3, 2023 22:14
roperzh pushed a commit that referenced this pull request Jan 16, 2023
…#9135)

Related to #9068 and
#8733, this builds on the work
done in #9062 to tie the loose ends
and assign the configured team to the device that's enrolling
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.

6 participants