Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Ingest Manager] Remove success: true as top-level API response #73223

Merged
merged 22 commits into from
Sep 2, 2020
Merged

[Ingest Manager] Remove success: true as top-level API response #73223

merged 22 commits into from
Sep 2, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Jul 25, 2020 •
edited
Loading

Summary

Implementation resulting from proposal/discussion at #73221

Key changes

Deleted top-level success property from all responses except

  1. /check-permissions (spec & handler). I figured we'd leave this as-is and make any changes in separate pass
  2. in array of objects returned by /package_configs/delete (spec & service)

Open issues/questions

There are a few endpoints where the only response was success: true leaving them empty now. I'm not sure how I, or others, feel about this.

  1. agent unenroll spec & handler
  2. agent reassign spec & handler

Some tests only looked for 200 code & success property 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 for item or some other expected property.

update: only server/routes/agent/{acks,actions}_handlers.test.ts tests reference the success property. IMO, removing it from them doesn't affect the tests other than to reflect the correct payload.

Big bang commit removing top-level success property in API response
666eaf6 
Left in check-permissions and in array of objects returned by delete package configs
@jfsiii jfsiii changed the title Big bang commit removing top-level success property in API response [Ingest Manager] Remove success: true as top-level API response Jul 25, 2020
@jfsiii jfsiii mentioned this pull request Jul 26, 2020
Closed
@jfsiii jfsiii self-assigned this Jul 26, 2020
@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 28, 2020

@elasticmachine merge upstream

@jfsiii jfsiii marked this pull request as ready for review July 28, 2020 18:09
@jfsiii jfsiii requested a review from a team as a code owner July 28, 2020 18:09
@jfsiii jfsiii requested a review from a team July 28, 2020 18:09
@jfsiii jfsiii requested a review from a team as a code owner July 28, 2020 18:09
@botelastic botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Jul 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii jfsiii added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Jul 28, 2020
@nchaulet
Copy link
Member

@michalpristas @blakerouse Are you using the success field in the agent?

@ph
Copy link
Contributor

ph commented Jul 28, 2020

There are a few places that we rely on Success but not much, we could remove it, most of the code already depends on HTTP code

elastic/beats#20288

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 28, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Jul 29, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 5, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 27, 2020

Failing tests are unrelated to this code. Watch #75808 for fix

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 27, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 27, 2020

@elasticmachine merge upstream

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 27, 2020

@ph @michalpristas CI is 💚

nchaulet
nchaulet approved these changes Aug 27, 2020
View reviewed changes
Copy link
Member

@nchaulet nchaulet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 27, 2020

@paul-tavares no worries at all. thanks for the 👀 Can you approve or ping the right people when you get a moment?

@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 31, 2020

@elasticmachine merge upstream

@jfsiii jfsiii requested review from a user and EricDavisX August 31, 2020 15:11
paul-tavares
paul-tavares approved these changes Aug 31, 2020
View reviewed changes
Copy link
Contributor

@paul-tavares paul-tavares left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

x-pack/plugins/ingest_manager/scripts/dev_agent/script.ts Outdated Show resolved Hide resolved
@jfsiii
Copy link
Contributor Author

jfsiii commented Aug 31, 2020

@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

jfsiii
jfsiii commented Aug 31, 2020
View reviewed changes
x-pack/plugins/ingest_manager/scripts/dev_agent/script.ts
@@ -122,7 +122,7 @@ async function enroll(kibanaURL: string, apiKey: string, log: ToolingLog): Promi
});
const obj: PostAgentEnrollResponse = await res.json();

if (!obj.success) {
if (!res.ok) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @paul-tavares

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 2, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
ingestManager 1.1MB -272.0B 1.1MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jfsiii jfsiii merged commit 71b9ded into elastic:master Sep 2, 2020
@jfsiii jfsiii mentioned this pull request Sep 2, 2020
Merged
jfsiii pushed a commit that referenced this pull request Sep 2, 2020
@elasticmachine
[Ingest Manager] Remove success: true as top-level API response (#73223…
b04c99d 
…) (#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>
@michalpristas michalpristas mentioned this pull request Sep 3, 2020
Merged
6 tasks
@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 4, 2020

@ph @ruflin can/should this go to 7.9.2? It's not a big conceptual/functional change, but it does have a pretty large surface area. I was hoping to avoid merge conflicts later with the many places that still have success: true in 7.9.

@ph
Copy link
Contributor

ph commented Sep 4, 2020

@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.

mdelapenya added a commit to mdelapenya/e2e-testing that referenced this pull request Sep 9, 2020
@mdelapenya
fix: "success" was remove from response
b29774c 
See elastic/kibana#73223
@mdelapenya mdelapenya mentioned this pull request Sep 9, 2020
Merged
8 tasks
mdelapenya added a commit to elastic/e2e-testing that referenced this pull request Sep 9, 2020
@mdelapenya
fix: "success" was removed from response (#278)
85231d2 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paul-tavares paul-tavares paul-tavares approved these changes

@michalpristas michalpristas michalpristas approved these changes

@nchaulet nchaulet nchaulet approved these changes

@EricDavisX EricDavisX Awaiting requested review from EricDavisX

Assignees

@jfsiii jfsiii

Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team v7.10.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jfsiii @elasticmachine @nchaulet @ph @paul-tavares @kibanamachine @michalpristas