[Ingest Manager] Remove success: true as top-level API response#73223
[Ingest Manager] Remove success: true as top-level API response#73223jfsiii merged 22 commits intoelastic:masterfrom jfsiii:73221-remove-success
Conversation
Left in check-permissions and in array of objects returned by delete package configs
|
@elasticmachine merge upstream |
|
Pinging @elastic/ingest-management (Team:Ingest Management) |
|
@michalpristas @blakerouse Are you using the |
|
There are a few places that we rely on |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
Failing tests are unrelated to this code. Watch #75808 for fix |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
|
@ph @michalpristas CI is 💚 |
|
@paul-tavares no worries at all. thanks for the 👀 Can you approve or ping the right people when you get a moment? |
|
@elasticmachine merge upstream |
paul-tavares
left a comment
There was a problem hiding this comment.
LGTM - had one question around the use of node-fetch and whether more code is needed to determine API error response.
Also - given the discussions today around upgrades and the possibility of pre-v7.10 agents communicating with a newer version of Kibana - is this a safe change for them? (I think you said it was not being used by the Agent, so we should be OK)
|
@paul-tavares, Agent did need some changes but "not much", iiuc. @michalpristas did a PR elastic/beats#20449 I'll let @michalpristas or @blakerouse comment re: auto upgrades |
| const obj: PostAgentEnrollResponse = await res.json(); | ||
|
|
||
| if (!obj.success) { | ||
| if (!res.ok) { |
|
@elasticmachine merge upstream |
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
…) (#76542) * Big bang commit removing top-level success property in API response Left in check-permissions and in array of objects returned by delete package configs * Remove success property from mocks * Resolve conflict from upstream changes * Remove success property (after upstream merge) * Remove more 'success'es after merging in upstream * Remove success from some tests * Remove success from OpenAPI spec * Revert prior try/catch. Use res.ok Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
|
@jfsiii I would prefer we do not backport that to 7.9.2. This has too many risks, we should keep 7.9.2 stable and really limit our backports to major issues. |
* fix: "success" was remove from response See elastic/kibana#73223 * chore: double max timeout when waiting for agent's events * chore: enrich error log message * chore: add a descrciptive comment about dates as strings
Summary
Implementation resulting from proposal/discussion at #73221
Key changes
Deleted top-level
successproperty from all responses except/check-permissions(spec & handler). I figured we'd leave this as-is and make any changes in separate pass/package_configs/delete(spec & service)Open issues/questions
There are a few endpoints where the only response was
success: trueleaving them empty now. I'm not sure how I, or others, feel about this.Some tests only looked for 200 code &
successproperty so deleting them means we only check for 200. e.g. https://github.com/elastic/kibana/pull/73223/files#diff-a1f7d4ab42c35079093986e3068feb23 Perhaps that's ok, or perhaps we could update them to look foritemor some other expected property.update: only
server/routes/agent/{acks,actions}_handlers.test.tstests reference thesuccessproperty. IMO, removing it from them doesn't affect the tests other than to reflect the correct payload.