-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
77133 check if transform exist before stopping and deleting #77640
77133 check if transform exist before stopping and deleting #77640
Conversation
Pinging @elastic/ingest-management (Team:Ingest Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approval so you can ship if you need to.
I left some notes, but since time is a factor and they're not difficult to change later, I'm fine with shipping this as-is if you want.
I have not checked this out and run locally yet. I'm doing that now.
@@ -33,89 +33,100 @@ export const installTransformForDataset = async ( | |||
registryPackage: RegistryPackage, | |||
paths: string[], | |||
callCluster: CallESAsCurrentUser, | |||
savedObjectsClient: SavedObjectsClientContract | |||
savedObjectsClient: SavedObjectsClientContract, | |||
logger: Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our current pattern is to have functions use the logger like const logger = appContextService.getLogger();
vs adding them to every function signature.
e.g.
kibana/x-pack/plugins/ingest_manager/server/services/epm/elasticsearch/ingest_pipeline/remove.ts
Line 19 in 46a268f
const logger = appContextService.getLogger(); |
@nnamdifrankie are the changes from 52a972c alone enough to prevent the 400's we saw? Not a big deal, but I'm curious about the effect the bad(?) paths have now |
I just checked this out locally and ran it against two different Cloud ES hosts that were failing. Both /setup calls returned |
There is no negative effect. I test both locally, integration test and against the cloud. It only make a difference I believe for remote host from what I can see. Where it is required because a fullpath may be given. |
@nnamdifrankie it's more than "no negative effect" it's the fix. At least in my tests. It's also consistent with the path format in the other elasticsearch/*/install.ts files 👍 to ship as soon as you're comfortable. I can deal with the logging & try/catch later Thanks again! |
💚 Build SucceededBuild metrics
History
To update your PR or re-run it, just comment with: |
…7640) [Ingest] fix transform path to work with external ES/cloud
…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
Summary
Checklist
Delete any items that are not applicable to this PR.features that require explanation or tutorials