Target intrinsic order

[iker-barriocanal]

Some targets should be run before others, to minimize harm in case one of them fails. The order should be determined by ascending impact/cost of reverting ratio. For example, registry should always be run last. We may also want to have an option to disable this order. How to approach this is open for discussion.

For context, see getsentry/sentry-javascript#4171.

[BYK]

This should be done in the project config itself as the order is deterministic already?

[rhcarvalho]

This should be done in the project config itself as the order is deterministic already?

The idea we've been discussing is that by default the order could be determined by Craft, regardless of the order in the config file.

This means we can make publish execute steps in the same predictable and "safer" order for all releases of all projects, regardless of the config files in the multiple repositories, and thus allowing us to configure the order in a single place -- Craft -- without changing every config file.

If we realize some project has a good reason to deviate from the intrinsic priority order, we could allow for the old behavior with a new config option or equivalent mechanism.

[BYK]

I strongly disagree with this approach, at least without keeping the old config. A better way forward then would be to adopt a dictionary of targets with clear relationships defined between them, like GitHub Actions' needs keyword and run all targets in parallel unless they need another one to finish.

This is a much better, future-proof and predictable way rather than taking away the execution order from project maintainers.

[iker-barriocanal]

How would you deal with half releases? If a target fails, we want to stop the release and don't continue with other targets.

[BYK]

@iker-barriocanal

How would you deal with half releases? If a target fails, we want to stop the release and don't continue with other targets.

You obviously won't execute any dependents when an upstream target fails. All others are allowed to finish. This is actually better as you publish to as many places as possible instead of many half-baked releases we shuffled to fix.

[iker-barriocanal]

@BYK there may be targets that are not dependant on each other but don't want to run them if one fails. For example, there aren't any dependencies between gcs, npm and github, but if one of them fails we don't want to run the others to prevent releasing to a subset of places. Does it mean we'd have to add dependants to avoid releasing to a subset of the targets?

[BYK]

but if one of them fails we don't want to run the others to prevent releasing to a subset of places. Does it mean we'd have to add dependants to avoid releasing to a subset of the targets?

Yes as you just defined what a dependency is? :D

[iker-barriocanal]

I understand, but that way will make every target depend on another target, which means executing targets linearly in a specific order and not parallelizing them. Is there something I'm missing?

[BYK]

@iker-barriocanal not every target. The current way, the ordered-list definition does what you say and we will keep that way of defining targets for backwards compatibility. What I'm proposing is a new type of target config with a dict where you can optionally set dependencies.

For instance, for the Craft releases themselves, I see on reason why npm, docker, and registry targets to wait for each other. Heck, even the github target can just happen anytime as it has its own artifact, the compiled binary. So, for Craft itself for instance, running all targets in parallel would be the more sensible solution. For any other project that wants to be more conservative, they can keep using the old list-style config or manually depend on things (such as registry depending on others, and github depending on registry etc.)

[antonpirker]

I think this issue is resolved. Also Iker and Rodolfo are not working at Sentry anymore, so it is safe to close this issue.

[BYK]

Don't think it is resolved but clearly we don't need it as badly for now so keeping closed :)