Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #16535 +/- ##
==========================================
+ Coverage 65.60% 65.79% +0.18%
==========================================
Files 1132 1138 +6
Lines 99069 100094 +1025
Branches 2448 2448
==========================================
+ Hits 64996 65852 +856
- Misses 29198 29330 +132
- Partials 4875 4912 +37
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
Overall looks great! Left some questions.
I've reviewed all non-test code. Am approving and will review tests tomorrow.
| err = fleetClient.DoGitOps(c.Context, config, baseDir, logf, flDryRun) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if flDryRun { | ||
| _, _ = fmt.Fprintf(c.App.Writer, "[!] gitops dry run succeeded\n") | ||
| } else { | ||
| _, _ = fmt.Fprintf(c.App.Writer, "[!] gitops succeeded\n") | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The issue says:
fleetctl gitops does a dry run first by default. If there's an error, show a helpful error message and don't make any changes to Fleet.
Shouldn't the non-dry run do a dry-run first?
There was a problem hiding this comment.
That comment refers to the GitHub action flow, which is the intended use case for fleetctl gitops. I kept the dry-run behavior consistent with how fleetctl apply works. FYI: @noahtalerman
| group.AppConfig = config.OrgSettings | ||
| group.EnrollSecret = &fleet.EnrollSecretSpec{Secrets: config.OrgSettings["secrets"].([]*fleet.EnrollSecret)} | ||
| group.AppConfig.(map[string]interface{})["agent_options"] = config.AgentOptions | ||
| delete(config.OrgSettings, "secrets") |
There was a problem hiding this comment.
Secrets are applied separately with Client.ApplyGroup, and it will trigger a validation fail if I don't delete them here.
| return ctxerr.Wrap(ctx, err, "getting app config") | ||
| } | ||
| if assumeEnabled { | ||
| appCfg.MDM.WindowsEnabledAndConfigured = true |
There was a problem hiding this comment.
What fails if you don't do this?
There was a problem hiding this comment.
Dry run fails when I try to enable and configure MDM at the same time.
|
In the user story it says:
What does it mean? (by remove all settings for that key?) |
|
On the demo I see that in some occasions you need to run Should this behavior be documented? |
lucasmrod
left a comment
lucasmrod
left a comment
There was a problem hiding this comment.
Ah, double checking something first:
Are you running fleetctl gitops as a user with GitOps role?
Or are we ok with running gitops with admin/maintainer roles? IIRC we use the gitops role for dogfood.
If user supplies empty policies, then we remove all policies. |
That shouldn't be the case. I think I run dry-run first, and then regular run. There is one MDM bug that caused me to run it twice: #16636 |
Yes, gitops role should run |
|
I will have a separate PR for gitops role: #16710 |
Add `fleetctl gitops` command for #13643 Code review video: https://www.loom.com/share/7941c51c709b44ccafd618dd05837d99?sid=27b923d7-1393-4396-bac7-30616b2d6de9 fleet-gitops PR that also needs review: fleetdm/fleet-gitops#26 Working global/team gitops configs that can be used for testing: https://github.com/fleetdm/fleet-gitops/tree/victor/fixing-configs # Checklist for submitter If some of the following don't apply, delete the relevant line. <!-- Note that API documentation changes are now addressed by the product design team. --> - [x] Changes file added for user-visible changes in `changes/` or `orbit/changes/`. - [x] Added/updated tests - [x] Manual QA for all new/changed functionality
2 participants
Add
fleetctl gitopscommand for #13643Code review video: https://www.loom.com/share/7941c51c709b44ccafd618dd05837d99?sid=27b923d7-1393-4396-bac7-30616b2d6de9
fleet-gitops PR that also needs review: fleetdm/fleet-gitops#26
Working global/team gitops configs that can be used for testing: https://github.com/fleetdm/fleet-gitops/tree/victor/fixing-configs
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/ororbit/changes/.