feat: move restartOnError from Actor to Run options

[DaveHanns]

Core: https://github.com/apify/apify-core/pull/23381
Worker: https://github.com/apify/apify-worker/pull/1515
Shared: apify/apify-shared-js#546
Py Client: apify/apify-client-python#508
Docs: apify/apify-docs#1965

[B4nan]

So the flag wasnt doing anything before? Technically this is a BC, but if it wasnt working at all, I guess its all right.

[B4nan]

Looking at the initial issue, it feels like it was working. Id rather keep it on the old place and mark it as deprecated. We can remove it when we will ship a major bump, together with removing support for older node versions and other things.

[DaveHanns]

Looking at the initial issue, it feels like it was working. Id rather keep it on the old place and mark it as deprecated. We can remove it when we will ship a major bump, together with removing support for older node versions and other things.

Actor interface is used only as a type of the returned Actor. After the migration, Actor will no longer have restartOnError at the top level, but rather in its defaultRunOptions. That is BC, but it should be reflected.

To not break existing API calls, we are going to support restartOnError top-level option in ActorUpdateOptions for a while (delegating the value to defaultRunOptions, with a metric set up to find out when it's no longer used), so I agree we might keep it there flagged as deprecated for a while as well.

[B4nan]

That is BC, but it should be reflected.

Not sure what you mean by that. If it's a BC, it should be reflected in a major bump of the library version.

Let's just mark it as deprecated, pointing to the correct place where it should be used.

[DaveHanns]

That is BC, but it should be reflected.

Not sure what you mean by that. If it's a BC, it should be reflected in a major bump of the library version.

Let's just mark it as deprecated, pointing to the correct place where it should be used.

Sorry for the confusion. What I meant is that leaving the flags at the top level of Actor create and update payload types is ok, as we will still accept the flags there for a while anyway.

restartOnError at top-level of Actor can be kept there and flagged deprecated, but it will never appear in the actual data returned to the user as after the migration, it will be permanently moved to the defaultRunOptions.

[B4nan]

but it will never appear in the actual data returned to the user as after the migration, it will be permanently moved to the defaultRunOptions.

In that case, we could handle this in the client code, moving the option to the right place if used on the deprecated one.

[B4nan]

Note that this sounds like you are planning to do a BC on the API level, which to me sounds like a bad idea on its own, the API should behave the same, we can't just juggle how the inputs look like. People might be using the API directly, not just via client, and we shouldn't break it just because we realized we did something wrong before.

[DaveHanns]

Note that this sounds like you are planning to do a BC on the API level, which to me sounds like a bad idea on its own, the API should behave the same, we can't just juggle how the inputs look like. People might be using the API directly, not just via client, and we shouldn't break it just because we realized we did something wrong before.

We will set up a tracking for whether the top-level resetOnError is used via API and until it is, we are going to support it. We are going to remove it only once we are sure there are no API calls using the top-level one.
That being said, we are assuming users set the resetOnError mainly via Console anyway, but we will find out.

The immediate BC is that we will no longer return the resetOnError at top level of the Actor, but rather in its deafultRunOptions. We can't really measure usage of that, but only 3% of the Actors have this flag set up and it will probably not be directly accessed, so the assumption is that this part of the change should be pretty safe.

We could, however, for at least some time, mirror the resetOnError at the top-level of the Actor, marking it deprecated just for those who actually use it there, to have time to adjust.

[DaveHanns]

Done. Top-level restartOnError is back, deprecated and we are going to remove it completely based on the tracking results in case of payloads and after few months (to let users adjust) in case of returned Actor.