SE: CLI setup experience changes #22956

mna · 2024-10-16T15:40:07Z

Checklist for submitter

Changes file added for user-visible changes in changes/, orbit/changes/ or ee/fleetd-chrome/changes.
See Changes files for more information.
Input data is properly validated, SELECT * is avoided, SQL injection is prevented (using placeholders for values in statements)
Added/updated tests
Manual QA for all new/changed functionality

codecov · 2024-10-21T19:56:11Z

Codecov Report

Attention: Patch coverage is 54.12844% with 150 lines in your changes missing coverage. Please review.

Please upload report for BASE (feat-setup-experience@aaea56d). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
server/service/client.go	51.85%	61 Missing and 17 partials ⚠️
server/service/client_scripts.go	12.90%	54 Missing ⚠️
server/datastore/mysql/software_installers.go	76.00%	4 Missing and 2 partials ⚠️
ee/server/service/teams.go	33.33%	3 Missing and 1 partial ⚠️
server/fleet/app.go	42.85%	3 Missing and 1 partial ⚠️
server/fleet/teams.go	55.55%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##             feat-setup-experience   #22956   +/-   ##
========================================================
  Coverage                         ?   63.13%           
========================================================
  Files                            ?     1546           
  Lines                            ?   146433           
  Branches                         ?     3644           
========================================================
  Hits                             ?    92451           
  Misses                           ?    46655           
  Partials                         ?     7327

Flag	Coverage Δ
backend	`64.00% <54.12%> (?)`

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.

mna · 2024-10-22T20:34:48Z

Note that there are no automated gitops tests for the new feature in this PR, I'll add those in a subsequent PR tomorrow (it has been manually tested). Also, setting the VPP apps to install during setup is not implemented for "No team" because VPP apps are not supported for "No team" at the moment - this ticket will track this (released AFAIK) bug: #22970

mna · 2024-10-23T15:59:48Z

Automated tests added in #23125

jahzielv · 2024-10-23T17:42:29Z

server/service/integration_mdm_test.go

@@ -11724,19 +11741,19 @@ func (s *integrationMDMTestSuite) TestSetupExperience() {
 		"GET", "/api/latest/fleet/software/titles",
 		listSoftwareTitlesRequest{},
 		http.StatusOK, &respListTitles,
-		"team_id", strconv.Itoa(int(team1.ID)),
+		"team_id", fmt.Sprint(team1.ID),


getvictor

Left some comments and questions, but can be merged as is.

getvictor · 2024-10-23T17:35:39Z

server/fleet/app.go

+	MacOSSetupAssistant         optjson.String                     `json:"macos_setup_assistant"`
+	EnableReleaseDeviceManually optjson.Bool                       `json:"enable_release_device_manually"`
+	Script                      optjson.String                     `json:"script"`
+	Software                    optjson.Slice[*MacOSSetupSoftware] `json:"software"`


Just curious, why does this need to be a pointer: *MacOSSetupSoftware?

Not strictly needed, but makes it easier to avoid issues as we need to change its field value here:

fleet/server/service/client.go

Line 858 in c11420b

sw.PackagePath = resolveApplyRelativePath(baseDir, sw.PackagePath)

A non-pointer would only change the local copy and would turn this into a potentially tricky bug to find.

getvictor · 2024-10-23T17:36:43Z

server/fleet/scripts.go

+	SelfService        bool   `json:"self_service"`
+	FleetMaintained    bool   `json:"-"`
+	Filename           string `json:"-"`
+	InstallDuringSetup *bool  `json:"install_during_setup"` // if nil, do not change saved value, otherwise set it


Why not use optjson.Bool here, and similar places?

In this case we don't need the extra features of optjson (i.e. no need to know if the key was explicitly set or not when it's null). optjson is mostly useful for PATCH-like processing where the key being absent is different than the key present but null.

getvictor · 2024-10-23T17:42:59Z

server/service/client.go

+		// this is a set of software packages or VPP apps that are configured as
+		// install_during_setup, by team. This is a gitops-only setting, so it will
+		// only be filled when called via this command.
+		tmSoftwareMacOSSetup := make(map[string]map[fleet.MacOSSetupSoftware]struct{}, len(tmMacSetup))


Nit. if tmMacSetup is empty, we shouldn't even need to call make

True, but I think it's minor enough not to matter here and makes the code a bit less verbose (and less error-prone if the conditions were to change).

getvictor · 2024-10-23T17:50:43Z

server/service/client.go

-		softwareInstallers, err = c.ApplyNoTeamSoftwareInstallers(payload, fleet.ApplySpecOptions{DryRun: dryRun})
+	}
+
+	// marshaling dance to get the macos_setup data


Nit: Needs better comment here. This is to get additional validation and get optjson functionality, right?

It's because we have it as a map[string]interface{} stored in a interface{} here, and hand-unwrapping all this is not super robust or easy to maintain. I'll update the comment to make that clearer.

getvictor · 2024-10-23T17:54:25Z

server/service/client_scripts.go

+	// planned, so to avoid delaying the feature I'm doing DELETE then SET, but
+	// that's not ideal (will always re-create the script when apply/gitops is
+	// run with the same yaml). Note though that we also redo software installers
+	// downloads on each run, so the churn of this one is minor in comparison.


Wouldn't get existing script and compare be better?

Please file a bug for this. This will cause setup experience to malfunction in some rare cases, right?

Wouldn't get existing script and compare be better?

Not necessarily, we'd be getting from the reader and loading/comparing a potentially large string to check for differences. I agree that it's worth creating a ticket, as gitops is all about "replace whatever's there" so we don't need to pay the price of a check. I'll file this bug.

This will cause setup experience to malfunction in some rare cases

I don't think so? I mean, there's a slight race between the delete and the insert if something else re-inserts first, but then it would fail applying it. I don't think it can really malfunction because of this, but it's still not ideal IMO.

mna temporarily deployed to Docker Hub October 16, 2024 15:40 — with GitHub Actions Inactive

mna temporarily deployed to Docker Hub October 16, 2024 18:39 — with GitHub Actions Inactive

mna temporarily deployed to Docker Hub October 16, 2024 20:47 — with GitHub Actions Inactive

mna added 7 commits October 21, 2024 08:50

Add macos setup fields to yaml struct

3853841

Adjust all fleetctl tests

1d0a09b

Fix missing software/scripts in fleetctl get teams in yaml format

003ae35

Add mdm software slice to cloner implementation

0c7cce7

Comment-out tests that cause OOM

8dd4e4f

Fix other service test

bdd60e5

Some pointers for implementation

25f48b2

mna force-pushed the mna-22385-cli-setup-experience branch from 4f893df to 25f48b2 Compare October 21, 2024 13:09

mna temporarily deployed to Docker Hub October 21, 2024 13:09 — with GitHub Actions Inactive

Add support to set the setup experience script for a team

fb2239c

mna temporarily deployed to Docker Hub October 21, 2024 15:42 — with GitHub Actions Inactive

Support matching software package by referenced yaml path

0684f23

mna temporarily deployed to Docker Hub October 21, 2024 19:35 — with GitHub Actions Inactive

Setting install_during_setup works for a team with software packages

05a4eaa

mna temporarily deployed to Docker Hub October 21, 2024 20:55 — with GitHub Actions Inactive

Software setup works for a team, not for no team

1f4bd19

mna temporarily deployed to Docker Hub October 22, 2024 13:22 — with GitHub Actions Inactive

Fix mysql tests

eb22853

mna had a problem deploying to Docker Hub October 22, 2024 14:01 — with GitHub Actions Error

Remove debug log

de0bae1

mna temporarily deployed to Docker Hub October 22, 2024 14:05 — with GitHub Actions Inactive

Fix integration test

22551a9

mna temporarily deployed to Docker Hub October 22, 2024 14:28 — with GitHub Actions Inactive

Add viaGitOps arg to ApplyGroup, use it to guard gitops-only features

bda9a2e

mna temporarily deployed to Docker Hub October 22, 2024 14:58 — with GitHub Actions Inactive

Make it all work for no team too

6142738

mna temporarily deployed to Docker Hub October 22, 2024 20:32 — with GitHub Actions Inactive

mna marked this pull request as ready for review October 22, 2024 20:50

mna requested a review from a team as a code owner October 22, 2024 20:50

Address unrelated lint issues

07bab9e

mna had a problem deploying to Docker Hub October 22, 2024 20:57 — with GitHub Actions Error

Fix stray paste

0ca593f

mna temporarily deployed to Docker Hub October 22, 2024 21:03 — with GitHub Actions Inactive

mna mentioned this pull request Oct 23, 2024

Install software and run setup script when Macs boot #19372

Open

47 tasks

mna added 2 commits October 23, 2024 09:20

Some more lint fixes

16da8a4

Fix conflict in schema

9881f06

mna had a problem deploying to Docker Hub October 23, 2024 13:27 — with GitHub Actions Error

More lint

07de1b6

mna temporarily deployed to Docker Hub October 23, 2024 13:35 — with GitHub Actions Inactive

mna mentioned this pull request Oct 23, 2024

SE: add CLI automated tests for fleetctl gitops #23125

Merged

1 task

Fix conflict

00c25a3

mna had a problem deploying to Docker Hub October 23, 2024 15:58 — with GitHub Actions Failure

Merge branch 'feat-setup-experience' into mna-22385-cli-setup-experience

2f0d46c

mna temporarily deployed to Docker Hub October 23, 2024 17:05 — with GitHub Actions Inactive

jahzielv reviewed Oct 23, 2024

View reviewed changes

getvictor approved these changes Oct 23, 2024

View reviewed changes

jahzielv approved these changes Oct 23, 2024

View reviewed changes

Merge branch 'feat-setup-experience' into mna-22385-cli-setup-experience

aed8653

mna had a problem deploying to Docker Hub October 23, 2024 18:27 — with GitHub Actions Error

Clarify comment for the marshal dance

779f998

mna temporarily deployed to Docker Hub October 23, 2024 18:30 — with GitHub Actions Inactive

mna mentioned this pull request Oct 23, 2024

fleetctl gitops: setup experience script does not have a "replace" endpoint #23149

Open

mna merged commit d1c3b5b into feat-setup-experience Oct 23, 2024
23 checks passed

mna deleted the mna-22385-cli-setup-experience branch October 23, 2024 18:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SE: CLI setup experience changes #22956

SE: CLI setup experience changes #22956

mna commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

mna commented Oct 22, 2024 •

edited

Loading

mna commented Oct 23, 2024

jahzielv Oct 23, 2024

getvictor left a comment

getvictor Oct 23, 2024

mna Oct 23, 2024

getvictor Oct 23, 2024

mna Oct 23, 2024

getvictor Oct 23, 2024

mna Oct 23, 2024

getvictor Oct 23, 2024

mna Oct 23, 2024

getvictor Oct 23, 2024

mna Oct 23, 2024

mna Oct 23, 2024

SE: CLI setup experience changes #22956

SE: CLI setup experience changes #22956

Conversation

mna commented Oct 16, 2024 • edited Loading

Checklist for submitter

codecov bot commented Oct 21, 2024 • edited Loading

Codecov Report

mna commented Oct 22, 2024 • edited Loading

mna commented Oct 23, 2024

Choose a reason for hiding this comment

getvictor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mna commented Oct 16, 2024 •

edited

Loading

codecov bot commented Oct 21, 2024 •

edited

Loading

mna commented Oct 22, 2024 •

edited

Loading