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

GitOps & API design: Add multiple Apple Business Manager and Volume Purchasing Program connections #21043

Merged
merged 54 commits into from
Sep 20, 2024

Conversation

noahtalerman
Copy link
Member

@noahtalerman noahtalerman commented Aug 5, 2024

GitOps and API changes for the following story:

DONE:

fleet-release
fleet-release previously approved these changes Aug 5, 2024
fleet-release
fleet-release previously approved these changes Aug 5, 2024

##### Example

```yaml
org_settings:
mdm:
apple_bm_default_team: "Workstations" # Available in Fleet Premium
apple_business_manager: # Available in Fleet Premium
Copy link
Member Author

@noahtalerman noahtalerman Aug 5, 2024

Choose a reason for hiding this comment

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

Dev note

Maintain support for the old apple_bm_default_team. If there's only one ABM token, this value populates the macos_team for the one ABM team.

When the user upgrades, set the new organization_name.macos_team to the value for the apple_bm_default_team.

If the user sets apple_bm_default_team and organization_name at the same time or they set apple_bm_default_team when there are more than one ABM tokens, return the following error:

"mdm.apple_bm_default_team has been deprecated. Please use the new mdm.apple_business_manager key documented here: https://fleetdm.com/learn-more-about/apple-business-manager-gitops"

Copy link
Member Author

Choose a reason for hiding this comment

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

fleet-release
fleet-release previously approved these changes Aug 5, 2024
Comment on lines -881 to -883
"apple_bm_default_team": "",
"apple_bm_terms_expired": false,
"enabled_and_configured": true,
Copy link
Member Author

@noahtalerman noahtalerman Aug 6, 2024

Choose a reason for hiding this comment

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

Dev note

Deprecate the apple_bm_default_team, apple_bm_terms_expired, apple_bm_enabled_and_configured flags (and remove them from docs) but support them for backwards compatibility.

What does support them mean?

  • apple_bm_default_team - If there's only one ABM token, during migration, this value populates the macos_team, ios_team, and ipados_team for the one ABM token. If there are more than one ABM tokens and the user tries to set apple_bm_default_team, return the following error: "mdm.apple_bm_default_team has been deprecated. Please use the new endpoint documented here: https://fleetdm.com/learn-more-about/apple-business-manager-teams-api"
  • apple_bm_terms_expired - set to true is there's one or more ABM tokens w/ terms expired
  • apple_bm_enabled_and_configured- set to true is there's one or more ABM tokens

Copy link
Member Author

Choose a reason for hiding this comment

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

mdm:
volume_purchasing_program: # Available in Fleet Premium
- location: Fleet Device Management Inc.
teams:
Copy link
Member Author

Choose a reason for hiding this comment

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

Dev note

If the user tries to add a team that doesn't exist, show the following error:

Couldn't edit org_settings.mdm.volume_purchasing_program. "💻 Workstations" team doesn't exist.

Copy link
Member Author

@noahtalerman noahtalerman Aug 6, 2024

Choose a reason for hiding this comment

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

QA note

The above error will likely happen when a user changes a team's name via GitOps but forgets to update the team name here.

Copy link
Member Author

@noahtalerman noahtalerman Aug 9, 2024

Choose a reason for hiding this comment

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

Dev note

If the user tries to add a team that already has a VPP token, show the following error:

Couldn't edit org_settings.mdm.volume_purchasing_program. "💻 Workstations" team already has a VPP token.  Each team can only have on VPP token.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @roperzh, now that a new VPP token doesn't have any teams by default (instead of "All team") how do you think the user should specify "All teams" via GitOps?

Could we make it so empty teams for a VPP token means it's available for "All teams." And once, the user adds a second VPP token we error and say you have to assign specific teams.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @noahtalerman that makes sense, the only gotcha I see is how would you define "no team"?

Copy link
Member Author

@noahtalerman noahtalerman Aug 12, 2024

Choose a reason for hiding this comment

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

I think they would specify "No team" which is a special, reserved team name in Fleet.

That made me wonder, what happens if I create a team w/ "No team" name in Fleet...

It actually breaks the team. And so does naming a team "All teams." Filed a bug for this here: #21264

Copy link
Member Author

Choose a reason for hiding this comment

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

@roperzh I think we want to add the reserved "All teams" and "No teams" teams as part of this story.

This way, the IT admin can specify these here in GitOps.

I think let's track this effort as part of the bug: #21264

What do you think?

noahtalerman and others added 4 commits September 19, 2024 17:00
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: Rachael Shaw <r@rachael.wtf>
noahtalerman and others added 3 commits September 19, 2024 17:01
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: Rachael Shaw <r@rachael.wtf>
Co-authored-by: Rachael Shaw <r@rachael.wtf>
@noahtalerman
Copy link
Member Author

@rachaelshaw please feel free to merge when the API doc changes look good to go!

Thank you for the review.

@jahzielv
Copy link
Contributor

jahzielv commented Sep 19, 2024

@noahtalerman

I would say for consistency's sake (we do return a similar error from the GET /api/latest/fleet/abm endpoint if there are > 1 ABM tokens) we should add this error.

Having a global default team doesn't seem make sense anymore now that we allow multiple tokens.

I can create a released bug if that works to track this?

@noahtalerman
Copy link
Member Author

I would say for consistency's sake (we do return a similar error from the GET /api/latest/fleet/abm endpoint if there are > 1 ABM tokens) we should add this error.

Having a global default team doesn't seem make sense anymore now that we allow multiple tokens.

I can create a released bug if that works to track this?

@jahzielv sounds good! Thanks for following up.

@noahtalerman
Copy link
Member Author

@lukeheath please feel free to merge this PR is the changes look good to you!

These reference docs are for the already released "Add multiple Apple Business Manager and Volume Purchasing Program connections" (#9956) story.

@lukeheath
Copy link
Member

@noahtalerman Thanks! Our goal is publish doc changes at the same time as the release. Let's evaluate this issue history together to determine where we need to improve process.

@lukeheath lukeheath merged commit 1677783 into main Sep 20, 2024
8 checks passed
@lukeheath lukeheath deleted the gitops-and-api-9956 branch September 20, 2024 17:21
@jahzielv
Copy link
Contributor

@noahtalerman sorry for the delay! Bug created for the missing error here: #22359

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.