Adding post create package callback

[CohenIdo]

Add a new call-back type that will be executed after the package policy is created.

[joshdover]

I'm curious if we could instead move the current callback to be executed after the package policy is created rather than before. It's current name even indicates it would be executed after: PostPackagePolicyCreateCallback.

@paul-tavares It looks like you added these back in #69428, do you remember why the hook is called before the package policy is created?

[paul-tavares]

@joshdover ,

Re:

I'm curious if we could instead move the current callback to be executed after the package policy is created rather than before. It's current name even indicates it would be executed after: PostPackagePolicyCreateCallback.

So the name is certainly confusing - the Post* word refers to the HTTP verb for the API operation.

For Endpoint, we do need an extension point before the package policy is saved. Thats because we need to retrieve the latest endpoint artifact manifest in order to include it in the package policy being created.

[joshdover]

Thanks, Paul. Makes sense.

Ido, I think it makes sense to proceed with this PR. Maybe we should change some of the naming and not use the word post anywhere to make this more clear.

[paul-tavares]

yeah, renaming would be good. names like PreCreate* and PostCreate* make more sense and are clearer in understanding when they are executed

[CohenIdo]

Hey @joshdover and @paul-tavares - thanks for your comments, I will take them into account while working on it.

[CohenIdo]

Hey @joshdover, @paul-tavares - updating that I added the new callback type packagePolicyPostCreate with the relevant tests.

For now, I leave the naming of the existing callback packagePolicyCreate as is.
I think it's better to manage this change in a different PR.

One known gap: I still didn't solve typescript issue that can be found also in the CI output

[nchaulet]

FYI Calling this in the handler means this will not work for preconfigured package policies, we already have the bug for other callbacks so it should probably not be addressed here #129383

[joshdover]

Great callout, agreed it'd be ok to defer on this and fix it for all callbacks in the same PR. That way they're all at least consistent.

[paul-tavares]

🤔 because the extension points are executed from the PackagePolicy service, I'm assuming the fix to the mentioned issue is to call the service method (runExternalCallbacks()) from the location where the preconfigured packages policies are added/updated while still leaving this code here in place.

[joshdover]

I think the idea would be to have packagePolicyService.create call runExternalCallbacks before returning the newly created policy to the caller. That way there is only a single call site that always runs regardless of how the package policy was created.

[elasticmachine]

Pinging @elastic/fleet (Team:Fleet)

[kfirpeled]

LGTM, fills our requirement: having integration's assets available on post-create callback

[paul-tavares]

Thanks for including me on this. The changes look good to me 👍 , but I defer approval to the fleet team on this.

[paul-tavares]

🤔 because the extension points are executed from the PackagePolicy service, I'm assuming the fix to the mentioned issue is to call the service method (runExternalCallbacks()) from the location where the preconfigured packages policies are added/updated while still leaving this code here in place.

[joshdover]

I think the idea would be to have packagePolicyService.create call runExternalCallbacks before returning the newly created policy to the caller. That way there is only a single call site that always runs regardless of how the package policy was created.

[CohenIdo]

Hey @joshdover, I re-request your review according to this change, I tried to use this scheme for validation and found that is not aligned with the PackagePolicy type.

export interface PackagePolicy extends Omit<NewPackagePolicy, 'inputs'> {
  id: string;
  inputs: PackagePolicyInput[];
  version?: string;
  revision: number;
  updated_at: string;
  updated_by: string;
  created_at: string;
  created_by: string;
}

[joshdover]

Thanks for the ping, this change makes sense. At first I thought that maybe the elasticsearch key should be duplicated / moved to CreatePackagePolicyProps as well but that would allow setting it on the API which we explicitly do not support. Everything else LGTM, thanks @CohenIdo

In the future, we may want to try refactoring this to derive the TS types from the config schema using the TypeOf helper from kbn/config-schema.

[CohenIdo]

Thanks for including me on this. The changes look good to me 👍 , but I defer approval to the fleet team on this.

@paul-tavares, @joshdover already approved, (I re-request his review again according to one more change), please have a look:)

[joshdover]

Thanks for the ping, this change makes sense. At first I thought that maybe the elasticsearch key should be duplicated / moved to CreatePackagePolicyProps as well but that would allow setting it on the API which we explicitly do not support. Everything else LGTM, thanks @CohenIdo

In the future, we may want to try refactoring this to derive the TS types from the config schema using the TypeOf helper from kbn/config-schema.

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: c213090
• Storybooks Preview
• Webpack Bundle Analyzer

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
fleet	1263	1267	+4

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
fleet	9	10	+1

Unknown metric groups

API count

id	before	after	diff
fleet	1380	1384	+4

History

• 💚 Build #37594 succeeded a03e724
• 💔 Build #37302 failed c113b13
• 💔 Build #37262 failed 11ac705
• 💔 Build #37233 failed 56bff9bd0e8fe9b147cdfa8dbe5db3378b47cfa5
• 💔 Build #37220 failed c637bc6
• 💔 Build #36261 failed a4a603343aed36d355377346e3dbaf0b356b1c61

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