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 remove teams #18640

Merged
merged 8 commits into from
May 3, 2024
Merged

GitOps remove teams #18640

merged 8 commits into from
May 3, 2024

Conversation

getvictor
Copy link
Member

@getvictor getvictor commented Apr 30, 2024

#16677

Improvements to fleetctl gitops command:

  • Added the ability to pass multiple files, like fleetctl gitops -f file1 -f file2, where the first file must be the global configuration
  • Added the ability to remove teams that were not specified in team configs using the switch --delete-other-teams
  • When passing a global config and team config during initial configuration, the org_settings.mdm.apple_bm_default_team value can be set to match the team that will be created by the provided team config.

After these changes are released to prod, we can update https://github.com/fleetdm/fleet-gitops to use the new switches: #18692

Checklist for submitter

If some of the following don't apply, delete the relevant line.

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

windowsEnabledAndConfigured := appCfg.MDM.WindowsEnabledAndConfigured
if opts.DryRunAssumptions != nil && opts.DryRunAssumptions.WindowsEnabledAndConfigured.Valid {
windowsEnabledAndConfigured = opts.DryRunAssumptions.WindowsEnabledAndConfigured.Value
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Sometimes, running dry run on team must assume that some setting(s) will be set by the global config. But they haven't actually been set yet, since we are doing a dry run.
I've created a new pattern to pass in such assumptions in the DryRunAssumptions struct.

@@ -1437,7 +1437,7 @@ type batchSetMDMProfilesRequest struct {
TeamID *uint `json:"-" query:"team_id,optional"`
TeamName *string `json:"-" query:"team_name,optional"`
DryRun bool `json:"-" query:"dry_run,optional"` // if true, apply validation but do not save changes
AssumeEnabled bool `json:"-" query:"assume_enabled,optional"` // if true, assume MDM is enabled
AssumeEnabled *bool `json:"-" query:"assume_enabled,optional"` // if true, assume MDM is enabled
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the setting so now we can assume disabled if AssumeEnabled != nil

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading this wrong, but isn't false a sensible default if no value exists?

Copy link
Member Author

Choose a reason for hiding this comment

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

The default value is appCfg.MDM.WindowsEnabledAndConfigured

With this change, if the global config disables Windows MDM, and a team tries to upload a profile, we will fail during dry run.

Copy link
Contributor

Choose a reason for hiding this comment

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

if the global config disables Windows MDM would the config be false or nil? Maybe I'm confused what AssumeEnabled == nil means.

@getvictor getvictor marked this pull request as ready for review May 1, 2024 21:48
@getvictor getvictor requested a review from a team as a code owner May 1, 2024 21:48
Copy link
Contributor

@mostlikelee mostlikelee left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple questions

Destination: &flFilename,
Usage: "The file with the GitOps configuration",
Destination: &flFilenames,
Usage: "The file(s) with the GitOps configuration. If multiple files are provided, the first file must be the global configuration and the rest must be team configurations.",
Copy link
Contributor

Choose a reason for hiding this comment

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

do we document anywhere the differences between a team config and global config? if not, it might be helpful add a short description. not sure it's always clear to users that global == nullTeam

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, we do not have documentation. GitOps is still considered in beta.

The plan is to document the GitOps config by replacing the current spec-based configs at https://fleetdm.com/docs/configuration/configuration-files
Product is responsible for updating those docs.

cmd/fleetctl/gitops.go Show resolved Hide resolved
@@ -1437,7 +1437,7 @@ type batchSetMDMProfilesRequest struct {
TeamID *uint `json:"-" query:"team_id,optional"`
TeamName *string `json:"-" query:"team_name,optional"`
DryRun bool `json:"-" query:"dry_run,optional"` // if true, apply validation but do not save changes
AssumeEnabled bool `json:"-" query:"assume_enabled,optional"` // if true, assume MDM is enabled
AssumeEnabled *bool `json:"-" query:"assume_enabled,optional"` // if true, assume MDM is enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be reading this wrong, but isn't false a sensible default if no value exists?

}
if appleBMDefaultTeam != "" && !appleBMDefaultTeamFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is it possible to move the team business logic out of the cli function to keep it shorter?

Copy link
Member Author

Choose a reason for hiding this comment

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

So move the AppleBM functions I created to spec package, or something more?

Copy link
Contributor

Choose a reason for hiding this comment

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

Team deletes below as well. Seems like most of the current logic is under DoGitOps()

@getvictor
Copy link
Member Author

@mostlikelee, I responded to your comments. No additional commits at this point.

Copy link
Contributor

@mostlikelee mostlikelee left a comment

Choose a reason for hiding this comment

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

Some suggestions, but LGTM

}
if appleBMDefaultTeam != "" && !appleBMDefaultTeamFound {
Copy link
Contributor

Choose a reason for hiding this comment

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

Team deletes below as well. Seems like most of the current logic is under DoGitOps()

@getvictor getvictor merged commit 4f4800b into main May 3, 2024
15 of 16 checks passed
@getvictor getvictor deleted the 16677-gitops-remove-teams branch May 3, 2024 13:03
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