ui: make silence creator and comment optional - #5471
Conversation
The API already accepts silences without a creator or a comment, but the web UI rejected the form unless both fields were filled. Drop the client-side non-empty validation so the form matches the API, label the fields as optional, and remove the now-unused stringNotEmpty helper. Signed-off-by: neoLsH <43921685+neoLsH@users.noreply.github.com>
Signed-off-by: neoLsH <43921685+neoLsH@users.noreply.github.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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe silence form no longer requires creator or comment values. Matcher and time validation remain required. Tests cover empty and populated fields and invalid times. ChangesSilence form validation
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The silence form now allows empty creator and comment fields while retaining validation for matchers and timing fields, matching the existing API behavior. No actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (1 skipped: 1 unsupported.) ✨ 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 |
SoloJacobs
left a comment
SoloJacobs
left a comment
There was a problem hiding this comment.
Hmm, the change here clearly does not full-fill #2998 as it was currently intended: The intent being to make Creator and Comment mandatory in the API. However, due to that being a breaking change, we will never change api v2 in that way.
I also don't think asserting that a field should be non-empty in the API is all that helpful: What users really want is that they can trace who silenced what and why.
Currently, the UI is doing a bit of work in that direction: It's a polite reminder that this information is important and useful. So, I think for some organizations this change might be a regression: Somebody might be too lazy to put the information in the UI, but they are probably are not malicious enough to craft an API response, just so they don't have to set the field.
Overall, I'm really undecided whether we should take this patch.
|
Thanks a lot for taking the time to review, and for the thoughtful perspective — this is really helpful for me as a first-time contributor here. You're right that I hadn't considered the full picture. When I picked up #2998, I focused narrowly on the UI/API consistency aspect, and since making the fields mandatory in api v2 isn't possible (as you pointed out, it would be a breaking change), relaxing the UI looked to me like the remaining way to resolve the inconsistency. I didn't think deeply enough about the role the current validation plays beyond validation — as a nudge that encourages people to leave traceability information ("who silenced what, and why"), which clearly has real value for organizations running Alertmanager in teams. I can see how removing it entirely could be a regression there, and I appreciate you pointing that out. I'd love to discuss what the best resolution is, and I'm happy to go in whatever direction the maintainers prefer. A few possibilities I can think of, in case any is useful:
Thanks again for the guidance. Just let me know which way you'd like to go, and I'll update (or close) the PR accordingly. |
|
It’s kind of strange that the ui check is used as a guardrail for otherwise a valid API request. I think it makes sense to align them one way or another. Also the non empty check is not doing that much of a job, the user can enter gibberish there to bypass validation. |
|
Hi @neoLsH , I appreciate the offer to help, but your PR already has the right shape, we just need to make a decision. I will ask a third maintainer, then we will see :-) Kind regards |
Signed-off-by: neoLsH <43921685+neoLsH@users.noreply.github.com>
|
This was discussed in the WG meeting. Consensus is:
|
3 participants
Pull Request Checklist
Which user-facing changes does this PR introduce?
The API already accepts silences without a
createdByor acomment(seeTestValidateSilence, where a valid silence carries neither), but the web UI rejected the silence form unless both fields were filled in. This made it impossible to create a silence from the UI that the API itself happily accepts.This change drops the client-side non-empty validation for the Creator and Comment fields so the form matches the API, labels both fields as "(Optional)", and removes the now-unused
stringNotEmptyhelper. Matchers, start/end time and duration are still validated as before.Verified locally with
make testinui/app(elm-format, elm-review and the elm-test suite all pass, including newtoSilence/validateFormtests) and a full UI build.