Skip to content

validate os updates and os profiles on dry-run for empty setup#47408

Open
MagnusHJensen wants to merge 2 commits into
mainfrom
dry-run-validate-os-updates-profile
Open

validate os updates and os profiles on dry-run for empty setup#47408
MagnusHJensen wants to merge 2 commits into
mainfrom
dry-run-validate-os-updates-profile

Conversation

@MagnusHJensen

@MagnusHJensen MagnusHJensen commented Jun 11, 2026

Copy link
Copy Markdown
Member

Related issue: Resolves #47364

Unreleased bug for 4.87

  • QA'd all new/changed functionality manually

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced validation to detect and prevent conflicting OS update configurations when both managed updates and custom configuration profiles attempt to manage OS updates.
  • Tests
    • Added automated tests covering detection of conflicts between managed OS updates and configuration profile contents for macOS/iOS and Windows.

Copilot AI review requested due to automatic review settings June 11, 2026 10:38
@MagnusHJensen MagnusHJensen requested a review from a team as a code owner June 11, 2026 10:38
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c488b8b6-2663-453b-934e-f5b1033eb57e

📥 Commits

Reviewing files that changed from the base of the PR and between b1f0022 and e2ca18c.

📒 Files selected for processing (1)
  • pkg/spec/gitops.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • pkg/spec/gitops.go

Walkthrough

This PR implements parse-time validation in GitOps file handling to detect and reject configurations where both managed OS updates and configuration profiles enforcing OS updates are simultaneously specified. The change adds a new validateOSUpdatesProfileConflict function that scans referenced Apple and Windows profile files for OS-update markers, compares them against active OS-update settings in GitOpsControls, and returns an error on conflict. This validation is integrated into the parseControls function to ensure consistent validation during both dry-runs and actual applies.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description links to issue #47364 but is minimal, with only a manual QA checkbox completed and no explanation of changes. Expand the description to briefly explain the validation logic added and confirm all relevant checklist items from the template are addressed.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly relates to the main change: validating OS updates and OS profiles during GitOps dry-run operations.
Linked Issues check ✅ Passed The code changes implement cross-field validation for OS-update conflicts in both macOS and Windows configurations during dry-runs, directly addressing issue #47364.
Out of Scope Changes check ✅ Passed All changes are directly scoped to validating OS-update and profile conflicts; no unrelated modifications were introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dry-run-validate-os-updates-profile

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copilot AI left a comment

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.

Pull request overview

This PR fixes a GitOps validation gap where fleetctl gitops --dry-run could succeed even when the YAML defines both managed OS-update settings and a conflicting OS-update configuration profile/declaration for the same fleet (a conflict that the server correctly rejects on non-dry-run apply). The change adds equivalent conflict detection during spec parsing so dry-runs fail early with the same underlying error message.

Changes:

  • Add dry-run-time validation to reject OS-update settings + OS-update profile/declaration conflicts for Apple and Windows controls.
  • Introduce helper functions to detect whether OS updates are configured and whether profile files contain OS-update enforcement markers.
  • Add unit tests covering failing and allowed combinations for Apple and Windows.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
pkg/spec/gitops.go Adds validateOSUpdatesProfileConflict plus helpers to detect OS-update settings and OS-update profiles during GitOps parsing (so dry-runs fail).
pkg/spec/gitops_test.go Adds tests that assert conflicts are rejected and non-conflicting cases remain allowed.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread pkg/spec/gitops.go
Comment thread pkg/spec/gitops.go Outdated
Comment thread pkg/spec/gitops_test.go
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 70.73171% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.19%. Comparing base (b4dcea8) to head (e2ca18c).

Files with missing lines Patch % Lines
pkg/spec/gitops.go 70.73% 6 Missing and 6 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #47408   +/-   ##
=======================================
  Coverage   67.19%   67.19%           
=======================================
  Files        3273     3273           
  Lines      227905   227946   +41     
  Branches    11746    11746           
=======================================
+ Hits       153137   153178   +41     
+ Misses      60956    60955    -1     
- Partials    13812    13813    +1     
Flag Coverage Δ
backend 68.84% <70.73%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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.

GitOps dry-run succeeds for a conflicting OS-update settings + profile config

2 participants