Bug 1876573 - Implement bitrisescript

[ahal]

No description provided.

[hneiva]

Looks great! 🚀
Only minor adjustments, so I'll pre-approve.

[bhearsum]

I think this largely looks fine. I'm marking it as Changes Requested due to the need to handle pagination. (If you think that's not needed immediately, I could be convinced.)

I also have a general question/concern: what do we want to do if a bitrisescript worker is terminated after a bitrise build is fired, but before it's completed? Do we want to try to cancel it as part of shutdown? Or do nothing and start a new build? Do we want the retried bitrisescript task to try to find it?

It seems like ideally we'd want to try to find the old bitrise build - but maybe that's a future optimization.

[JohanLorenzo]

Thanks for converting the existing script into a *script one! This is much better 👍 This looks good to me. I agree with the points raised by @bhearsum, I leave you two agree on the best approach 🙂

[ahal]

I implemented pagination, added a more specific Exception class, switched to a single TOKEN variable and fixed a bug around unclosed aiohttp sessions.

[ahal]

I also have a general question/concern: what do we want to do if a bitrisescript worker is terminated after a bitrise build is fired, but before it's completed? Do we want to try to cancel it as part of shutdown? Or do nothing and start a new build? Do we want the retried bitrisescript task to try to find it?

It seems like ideally we'd want to try to find the old bitrise build - but maybe that's a future optimization.

I missed this question! Good call out.. I'm not sure how we could find an already in-progress build, and I'm assuming we'd only want to find existing builds if there was a spot termination or some other infra error (e.g, not user cancel). So given those two facts, I'd lean towards just always attempting to cancel the build in Bitrise.

Remind me.. do we just need to handle SIGTERM? If it's alright with you, I think I'd like to tackle this in a separate PR as there will be a few layers to this (due to needing to handle SIGTERM and then cancel each future and then have each future cancel the build via Bitrise API). I'll file an issue and tackle it soon.

[ahal]

See #924 for cancel feature

[bhearsum]

I also have a general question/concern: what do we want to do if a bitrisescript worker is terminated after a bitrise build is fired, but before it's completed? Do we want to try to cancel it as part of shutdown? Or do nothing and start a new build? Do we want the retried bitrisescript task to try to find it?
It seems like ideally we'd want to try to find the old bitrise build - but maybe that's a future optimization.

I missed this question! Good call out.. I'm not sure how we could find an already in-progress build, and I'm assuming we'd only want to find existing builds if there was a spot termination or some other infra error (e.g, not user cancel). So given those two facts, I'd lean towards just always attempting to cancel the build in Bitrise.

That sounds fine to me. If we get to a point where we're wasting a bunch of credits because of this we can always reconsider.

Remind me.. do we just need to handle SIGTERM?

Honestly, I don't know...but it seems likely?

If it's alright with you, I think I'd like to tackle this in a separate PR as there will be a few layers to this (due to needing to handle SIGTERM and then cancel each future and then have each future cancel the build via Bitrise API). I'll file an issue and tackle it soon.

Yup, this is perfectly fine. Just wanted to make sure it was thought about.