previously there was --format=json, can these be used interchangeably? Or the previous one was not working correctly? :)

same behaviour. Its a bash way of defining "long" parameter values

not sure if needed?

this will be deleted in my next PR. I only used the code below to test that we could call the external API during my test.

👍

why do we need this metaprogramming here? I guess it can be useful in some cases, e.g. if we want to add logging to all methods of a class instead of modifying all of its methods one by one, but here we're just adding an additional layer of complexity to be able to modify one function, if I understood correctly.

could you consider passing the background task related parameters to the function processPendingActions() instead? or maybe adding those parameters to the ConstructorOptions of getResponseActionsClient(), if we want to store those params for the lifetime of the responseActionsClient object?

I did not want to introduce Task specific details all the way down to the Action Response client - I feel that would just add unnecessary complexity. The data is only needed to silence a .warn()'ing that this new Connector's sub-actions client's .execute() method outputs when this data is not set. So its needed for all calls to .execute() from the Connector's sub-actions framework - thus I wanted to keep it in a place that would apply only to the use of this client (note: the sub-actions client that is used from our HTTP apis for response actions uses a different Connector client that does not have this need).

The use of the Connector sub-actions is an internal details to the implementation of the Response Actions Client's, thus I also did not want to expose connector specific details on our class methods and rather keep them as "pure" as possible as far as input arguments go. This will also makes it cleaner should we ever want to make internal changes to how we communicate with external systems by not having to alter our exposed interfaces and associated method signatures.

I also considered changing the constructor arguments to instead accept on input an instance of the new Class I created (NormalizedExternalConnectorClient), but because that class needs the agentType I felt it would be best for it to continue to be used only from within the ResponseActionsClient sub-classes where the agentType is already know.

Let me know what you think. I can still evaluate to see if there is a more clean and transparent way to do this without having to expose this data down/up the chain of the code.

I see your point, if it's only for addressing a warning, and there is no change in the behaviour, I guess we can live with that.

Still, I have a feeling that when we finally call execute(), it receives taskId+taskType and requesterId: 'background_task' parameters, which seem related to me, but they come from 2 separate paths:

• taskId+taskType come from proxying the connector client which happens only for non-endpoint agents in EndpointAppContextService,
• 'background_task' comes from SentinelOneActionsClient when it calls execute()

So, a group of related parameters, that are set together accidentally. If my assumption is right, I'd suggest to simplify the dataflow by putting them in one place, either in SentinelOneActionsClient, or if it's really only the warning, in the Proxy.
If they are not related, feel free to skip my comment. :)

I tried to depict the situation in a flowchart-like irregular chart, where I marked the non-endpoint specific stuff with red.

flowchart TD

taskRunner(["CompleteExternalActionsTaskRunner.run()"])-->|taskId| getInternalResponseActionsClient(["getInternalResponseActionsClient()"])
getInternalResponseActionsClient --> getResponseActionsClient
getInternalResponseActionsClient--- |taskId| connectorsClient
connectorsClient --o |"taskId, requesterId: 'background_task'"| execute

getResponseActionsClient(["getResponseActionsClient()"]) --> SentinelOneActionsClient
SentinelOneActionsClient ---|"requesterId: 'background_task'"| NormalizedExternalConnector
NormalizedExternalConnector--o |"requesterId: 'background_task'"| connectorsClient

taskRunner --> processPendingActions(["processPendingActions()"])
processPendingActions -.-> execute(["`execute( *taskId, 'background_task'* )`"])

style SentinelOneActionsClient stroke:#d55,stroke-width:4px
style NormalizedExternalConnector stroke:#d55,stroke-width:4px
style taskRunner stroke:#d55,stroke-width:4px

How about this: I'll make some changes to the NormalizedExternalConnector class and will add these parameters to it instead and will then use that class instance to initialize the Response Actions Client's.

Sound good to you?

But actually - I will do that in a follow up PR, if thats ok. Need to get this in so I can use it in my other PR and Tomasz also needs the changes

sure, go for it! don't want to hold up the work, so follow up PR is perfect

yeah, using the NormalizedExternalConnector sounds reasonable, let's see 👍