fix: allow empty string in RC apply_error#7020
Conversation
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: b809c9c | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b809c9cc39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| error = state.get("apply_error", "") # Allow unset or empty string | ||
| if error != "": |
There was a problem hiding this comment.
This change allows any config_state with apply_error == "" to pass, but the RFC contract (also quoted in this commit message) only permits empty/unset apply_error when apply_state is not ERROR. In test_no_error, a payload like {apply_state: "ERROR", apply_error: ""} will now be treated as success, so a real remote-config application failure can be missed by the test suite. Please gate the empty-string allowance on apply_state and still fail when the state is ERROR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
if the value is present, and is null, is it valid ?
There was a problem hiding this comment.
not explicitely specified, we can add it to the list of other libraries start failing tests because of it
MUST contain an associated error message if apply_state is ERROR.
MUST NOT be set OR empty string if apply_state is anything else.
2 participants
Motivation
According to RFC
RCTE2 - Configuration apply statuswhich defines the individual RC configs error states, theapply_errorfield explicitly consider empty string to be a nominal case:Changes
Allow
apply_errorto be either unset or empty string to be considered nominal.