jira: add labels.update_mode replace|merge for label updates - #5443
Open
Drenovik wants to merge 3 commits into
Open
jira: add labels.update_mode replace|merge for label updates#5443Drenovik wants to merge 3 commits into
Drenovik wants to merge 3 commits into
Conversation
Support object-form labels with enable_update while keeping the legacy YAML list fully compatible. When enable_update is false, omit labels on PUT so manual/automation labels are not wiped (create still sets labels). Signed-off-by: Drenovik <forget.alphabet071@slmails.com>
Adds an opt-in merge mode that appends labels via Jira's update.labels add operations instead of replacing fields.labels wholesale, so labels added manually or by automation are preserved. Default remains replace (today's behavior); enable_update: false still wins over update_mode. Signed-off-by: Drenovik <forget.alphabet071@slmails.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesJira label updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Alertmanager
participant JiraNotifier
participant JiraAPI
Alertmanager->>JiraNotifier: Process notification
JiraNotifier->>JiraAPI: Create issue with configured labels
JiraNotifier->>JiraAPI: Update issue
alt Updates disabled
JiraNotifier->>JiraAPI: Send fields without labels
else Merge mode
JiraNotifier->>JiraAPI: Send update.labels add operations
else Replace mode
JiraNotifier->>JiraAPI: Send fields with labels
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 (3)
notify/jira/types.go (1)
36-39: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDrop
omitemptyonlabelOp.Add.
addis the operation payload, not an optional attribute. If a label template renders to an empty string,omitemptyserializes the element as{}, which Jira rejects as an invalid update operation. Withoutomitemptythe request carries{"add": ""}, which produces a clearer Jira validation error.♻️ Proposed change
type labelOp struct { - Add string `json:"add,omitempty"` + Add string `json:"add"` }🤖 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 `@notify/jira/types.go` around lines 36 - 39, Update the Add field in the labelOp struct to remove the json omitempty option, ensuring empty label values serialize as an explicit add payload rather than an empty object.notify/jira/jira_test.go (1)
1716-1724: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove or use the
wantEnableUpdateNilfield.No table case sets
wantEnableUpdateNil, so the assertion at Line 1796 never runs. Either delete the field, or set it totruefor the cases whereenable_updateis omitted.♻️ Proposed change
for _, tc := range []struct { - name string - yaml string - wantValues []string - wantEnableUpdate bool - wantEnableUpdateNil bool - wantUpdateMode JiraLabelUpdateMode - wantErr string + name string + yaml string + wantValues []string + wantEnableUpdate bool + wantUpdateMode JiraLabelUpdateMode + wantErr string }{Then drop the
if tc.wantEnableUpdateNilblock at Lines 1796-1798.🤖 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 `@notify/jira/jira_test.go` around lines 1716 - 1724, Remove the unused wantEnableUpdateNil field and its conditional assertion block in the table-driven test, since no test case sets it and the nil-state assertion never executes.notify/jira/config.go (1)
115-132: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStart the exported method comment with the identifier name.
The comment on
UnmarshalYAMLdoes not begin with the method name. The repository lints Go comments with godot and revive. Align the comment with the convention used by the other exported identifiers in this file.Also consider validating
UpdateModeinJiraConfigvalidation as well.UnmarshalYAMLis the only place that rejects an unknown mode. AJiraLabelsConfigbuilt programmatically or decoded from JSON keeps the invalid value, andUpdateModeValuethen returns it unchanged, whichjira.gotreats asreplace.♻️ Proposed comment fix
-// Supports both the legacy list and the new object form. +// UnmarshalYAML supports both the legacy list form and the new object form. func (l *JiraLabelsConfig) UnmarshalYAML(unmarshal func(any) error) error {🤖 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 `@notify/jira/config.go` around lines 115 - 132, Update the exported comment for JiraLabelsConfig.UnmarshalYAML so it begins with “UnmarshalYAML”. Also extend JiraConfig validation to reject JiraLabelsConfig.UpdateMode values other than empty, JiraLabelUpdateReplace, or JiraLabelUpdateMerge, covering programmatic and JSON-created configurations consistently with UnmarshalYAML.Source: Coding guidelines
🤖 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 `@notify/jira/config.go`:
- Around line 115-132: Update the exported comment for
JiraLabelsConfig.UnmarshalYAML so it begins with “UnmarshalYAML”. Also extend
JiraConfig validation to reject JiraLabelsConfig.UpdateMode values other than
empty, JiraLabelUpdateReplace, or JiraLabelUpdateMerge, covering programmatic
and JSON-created configurations consistently with UnmarshalYAML.
In `@notify/jira/jira_test.go`:
- Around line 1716-1724: Remove the unused wantEnableUpdateNil field and its
conditional assertion block in the table-driven test, since no test case sets it
and the nil-state assertion never executes.
In `@notify/jira/types.go`:
- Around line 36-39: Update the Add field in the labelOp struct to remove the
json omitempty option, ensuring empty label values serialize as an explicit add
payload rather than an empty object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c08fc4-f476-4ffe-9569-0ef06c1a961d
📒 Files selected for processing (8)
CHANGELOG.mdconfig/config_test.goconfig/testdata/conf.jira-legacy-labels.ymldocs/configuration.mdnotify/jira/config.gonotify/jira/jira.gonotify/jira/jira_test.gonotify/jira/types.go
Signed-off-by: Drenovik <forget.alphabet071@slmails.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #5442
What changed
UpdateModeValue(), and validation rejecting any other value.
labelOp{Add string} to represent Jira's update.labels: [{add: ...}] operation.
labels are sent as update.labels add-operations instead of a fields.labels SET, and
fields.labels is omitted from that request. replace (default) behavior is unchanged
from today. enable_update: false still wins over update_mode (skip takes priority
over merge).
raw JSON map; reworked it to unmarshal into the typed issue{} struct so
Fields/Update are structurally asserted, not just spot-checked.
merge/replace/skip interactions, and raw-JSON checks that fields.labels and
update.labels are never both present in one request body.
Why
enable_update: false (this PR's predecessor) fully stops label updates, which is
sometimes too blunt - users may still want Alertmanager's templated labels applied on
update, while preserving labels added by humans/automation. update_mode: merge
achieves this using Jira's native incremental label API instead of a destructive
replace.
Design notes / limitations
Alertmanager template between updates. This is called out explicitly in the docs -
a "smart sync" (diffing old vs new label sets) was considered and rejected as
unnecessary complexity for this use case.
are skipped entirely, not merged.
the object form to opt in.
Testing
harness rework asserting typed Fields/Update equality, plus new cases for
merge/replace/skip combinations and unmarshal validation of update_mode.
issue with update_mode: merge, manually added a foreign label, triggered an update,
and confirmed both the Alertmanager-managed labels were present and the manually
added label survived (add-only semantics behaved as intended against a real Jira
API, not just a test double). Also re-verified the enable_update: false path from
the predecessor PR still works unchanged with this code present, and exercised
several edge cases (empty label list, idempotent repeated updates, a label value
rejected by Jira's own validation) to confirm no partial/corrupted writes occur in
any case.
Pull Request Checklist
Please check all the applicable boxes.
Which user-facing changes does this PR introduce?