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] Rollback changes from #77640 not related to the fix #77806

Merged
merged 1 commit into from
Sep 18, 2020
Merged

[Ingest Manager] Rollback changes from #77640 not related to the fix #77806

merged 1 commit into from
Sep 18, 2020

Conversation

jfsiii
Copy link
Contributor

@jfsiii jfsiii commented Sep 17, 2020 •
edited
Loading

Summary

The key to #77640 was adding the leading slash to the paths and checking for the transform before we delete it. Those changes are not affected by this PR.

This rolls back any changes that weren't a part of the resolution and/or aren't consistent with plugin style.

  • Doing a try/catch and re-throwing doesn't gain us anything. The error is already caught in the route handler
  • We have logging for the issue in the existing handler. Where we do use logging we import from appContextService.getLogger() vs supplying as a parameter

Rollback the logger & try/catch changes from elastic#77640
fdcaa3a 
 * Doing a try/catch and re-throwing doesn't gain us anything. We already catching the error in the route handler
 * We have logging for the issue in the existing handler. We also don't pass a logging context to functions
@jfsiii jfsiii added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.10.0 Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 17, 2020
@jfsiii jfsiii requested a review from a team September 17, 2020 18:52
@jfsiii jfsiii self-assigned this Sep 17, 2020
@elasticmachine
Copy link
Contributor

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

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

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

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 18, 2020

cc @neptunian

@neptunian
Copy link
Contributor

This looks good but I'm not seeing in #77640 where we check the transform exists before deleting it or catch the the possible delete errors.

@jfsiii
Copy link
Contributor Author

jfsiii commented Sep 18, 2020 •
edited
Loading

Perhaps I mischaracterized what #77640 does, but it does solve the original issue from what we've seen. I believe this is the check aeade44#diff-f05d5ef07fe6c504f052b00bfbdb63a7 but looking at the final commit https://github.com/elastic/kibana/pull/77640/files it seems that's no longer in x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/transform/remove.ts which seems to further the idea that the problem was always the bad paths.

This PR doesn't change any behavior. It only deletes the try/catch and log statement from whatever was added.

@jfsiii jfsiii merged commit 65387d4 into elastic:master Sep 18, 2020
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 22, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 77806 or prevent reminders by adding the backport:skip label.

@jfsiii jfsiii mentioned this pull request Sep 22, 2020
Merged
jfsiii pushed a commit to jfsiii/kibana that referenced this pull request Sep 22, 2020
Rollback the logger & try/catch changes from elastic#77640 (elastic#7…
22f2357 
…7806)

* Doing a try/catch and re-throwing doesn't gain us anything. We already catching the error in the route handler
 * We have logging for the issue in the existing handler. We also don't pass a logging context to functions
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Sep 23, 2020
jfsiii pushed a commit that referenced this pull request Sep 23, 2020
[7.x] Rollback the logger & try/catch changes from #77640 (#77806) (#…
32d131a 
…78197)

* Rollback the logger & try/catch changes from #77640 (#77806)

* Doing a try/catch and re-throwing doesn't gain us anything. We already catching the error in the route handler
 * We have logging for the issue in the existing handler. We also don't pass a logging context to functions

* Add missing import
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neptunian neptunian neptunian approved these changes

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.
4 participants
@jfsiii @elasticmachine @kibanamachine @neptunian