Warn when labels key appears in no-team/unassigned GitOps files - #44865
Conversation
Closes #42522. The labels: top-level key in no-team.yml or unassigned.yml was silently accepted. Now it logs a warning and ignores the key, matching the existing pattern for agent_options and reports.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughGitOpsFromFile now marks a top-level 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/spec/gitops_test.go (1)
2417-2437: ⚡ Quick winConsider adding the same assertion for
unassigned.ymlpath.The parser path also treats
unassigned.ymlas no-team via coercion, so a sibling test would guard that branch and warning text (unassigned.yml) against regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/spec/gitops_test.go` around lines 2417 - 2437, Extend TestLabelsIgnoredInNoTeamFile to also verify the parser behavior for an "unassigned.yml" filename: create a second temp file via createNamedFileOnTempDir using the same config but named "unassigned.yml", call GitOpsFromFile with that unassignedPath/unassignedBasePath (reusing the captureLogf/logMessages setup), assert LabelsPresent is false and Labels is empty for the returned gitops, and assert the logged warning contains "'labels' is not supported in unassigned.yml" to cover the coerced no-team branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/spec/gitops_test.go`:
- Around line 2417-2437: Extend TestLabelsIgnoredInNoTeamFile to also verify the
parser behavior for an "unassigned.yml" filename: create a second temp file via
createNamedFileOnTempDir using the same config but named "unassigned.yml", call
GitOpsFromFile with that unassignedPath/unassignedBasePath (reusing the
captureLogf/logMessages setup), assert LabelsPresent is false and Labels is
empty for the returned gitops, and assert the logged warning contains "'labels'
is not supported in unassigned.yml" to cover the coerced no-team branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e24c95e8-1768-46c2-b8a3-4cd58895371c
📒 Files selected for processing (2)
pkg/spec/gitops.gopkg/spec/gitops_test.go
There was a problem hiding this comment.
Pull request overview
This PR updates GitOps file parsing to handle a labels: top-level key in no-team/unassigned GitOps YAMLs by warning and ignoring the key, and adds a unit test to validate the behavior.
Changes:
- Log a warning and skip label parsing when
labels:appears in no-team/unassigned GitOps files. - Add a unit test to ensure labels are not parsed and that a warning is emitted.
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 an IsNoTeam() guard around label parsing and logs a warning when labels: is present in no-team/unassigned files. |
| pkg/spec/gitops_test.go | Adds TestLabelsIgnoredInNoTeamFile to validate warning + ignore behavior for labels: in a no-team file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44865 +/- ##
==========================================
+ Coverage 66.66% 66.79% +0.12%
==========================================
Files 2672 2678 +6
Lines 214802 216894 +2092
Branches 9946 9946
==========================================
+ Hits 143206 144877 +1671
- Misses 58562 58848 +286
- Partials 13034 13169 +135
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/spec/gitops_test.go`:
- Around line 2431-2433: The test currently asserts that gitops.LabelsPresent is
true for no-team files, but for unsupported/ignored labels in no-team mode it
should be false; update the assertion in the test checking gitops.LabelsPresent
to assert false (keep the following assert.Empty(t, gitops.Labels) unchanged) so
the test correctly guards against parsing labels in no-team files.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 511354af-6261-4053-a540-37a9545ca14f
📒 Files selected for processing (1)
pkg/spec/gitops_test.go
Address review feedback:
- Return an error (via multiError) instead of just logging a warning,
matching the issue requirement to "fail with an error"
- Set LabelsPresent = true unconditionally when the key exists, keeping
its semantics consistent ("key was in the YAML")
- Add unassigned.yml sub-test alongside no-team.yml
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/spec/gitops.go (1)
523-530: 💤 Low valueBehavioral asymmetry between
labelsand other no-team-unsupported keys
parseAgentOptions(lines 884–889) andparseReports(lines 1727–1731) silently log a warning and proceed for no-team files, whilelabelsnow appends a hard error that aborts the run. This meansagent_options:orreports:in a no-team file produces a non-fatal warning butlabels:causes a CI failure. This is intentional per the post-review commit message, but a future caller may want to standardize the treatment of all unsupported keys (error vs warning).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/spec/gitops.go` around lines 523 - 530, The handling of 'labels' is inconsistent: when result.IsNoTeam() the code currently appends a hard error, while parseAgentOptions and parseReports only log a warning; update the labels branch in the if block that sets result.LabelsPresent (around parseLabels) so that for no-team files it mirrors the other handlers — call logFn to emit a warning (include filepath.Base(filePath) and context) and do not append to multiError; leave the else path to call parseLabels as before. Reference: result.IsNoTeam(), result.LabelsPresent, parseLabels, logFn, and filePath.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/spec/gitops.go`:
- Around line 523-530: The handling of 'labels' is inconsistent: when
result.IsNoTeam() the code currently appends a hard error, while
parseAgentOptions and parseReports only log a warning; update the labels branch
in the if block that sets result.LabelsPresent (around parseLabels) so that for
no-team files it mirrors the other handlers — call logFn to emit a warning
(include filepath.Base(filePath) and context) and do not append to multiError;
leave the else path to call parseLabels as before. Reference: result.IsNoTeam(),
result.LabelsPresent, parseLabels, logFn, and filePath.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6116cdfc-c264-46da-8c7c-91c0b6438807
📒 Files selected for processing (2)
pkg/spec/gitops.gopkg/spec/gitops_test.go
|
Re: CodeRabbit nitpick on behavioral asymmetry between The asymmetry is intentional. The ticket (#42522) explicitly requires an error: "Fail with an error like we do for any other unsupported top-level key." A hard error is also the better UX here — silently ignoring |
|
Changes LGTM, you are missing the changes file |
Use a warning (logFn) instead of a hard error to avoid breaking existing customer GitOps pipelines that happen to have labels: in their no-team file. This matches the existing pattern used by agent_options and reports. Also adds the changelog file.
|
@sgress454 — After thinking about this more, I'm switching from a hard error to a warning. Customers with existing |
sharon-fdm
changed the title
|
@juan-fdz-hawa , changelog added. |
juan-fdz-hawa
left a comment
juan-fdz-hawa
left a comment
There was a problem hiding this comment.
We should standardize how we handle grammar violations like this. My understanding is that we should throw an error rather than a warning, as users tend to ignore warnings (though the First Impressions team may have the final say here). The code changes themselves look good, so I'm approving this for now.
Closes #42522
Changes
When
labels:appears in a no-team/unassigned GitOps file, log a warning and skip label parsing. This matches the existing pattern used byagent_optionsandreportsin no-team files.A warning (not an error) is used intentionally to avoid breaking existing customer GitOps pipelines that may already have
labels:in their no-team file.After fix:
Testing
Manual testing
Built
fleetctlfrom the fixed branch against a local Fleet server (premium license).unassigned.ymldry-rununassigned.ymlreal runno-team.ymldry-runno-team.ymlreal rununassigned.ymlwithout labelsUnit tests
TestLabelsIgnoredInNoTeamFile: Sub-tests for bothno-team.ymlandunassigned.ymlassert: (1) no error, (2)LabelsPresentis true, (3) no labels parsed, (4) warning logged.