-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1876573 - Implement bitrisescript #922
Conversation
b3f0b0c
to
52dcac1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🚀
Only minor adjustments, so I'll pre-approve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
b2369f9
to
aa57636
Compare
Bitrisescript will be used to interface with Bitrise's API. In this initial version, only the ability to trigger Bitrise workflows will be added, though more capabilities may be added in the future. Usage of bitrisescript will require at least two scopes: - <prefix>:app:<app> - <prefix>:workflow:<workflow> The first will ensure the task has permission to trigger workflows for the specific app in Bitrise. The second will ensure the task has permission to trigger a specific workflow. Tasks may specify multiple workflow scopes, which will trigger each workflow asynchronously.
I implemented pagination, added a more specific Exception class, switched to a single TOKEN variable and fixed a bug around unclosed aiohttp sessions. |
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. |
See #924 for cancel feature |
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.
Honestly, I don't know...but it seems likely?
Yup, this is perfectly fine. Just wanted to make sure it was thought about. |
No description provided.