Separate API calls from install logic #29
Conversation
Nextflow binary URLs should be stored in the new NextflowRelease object, so it should essentially be private for the NextflowRelease type. Move the function to that file to signify that (but retain export for testing purposes).
WIth the goal being to convert Octokit data into NextflowRelease objects, create a function that can do that in one line.
Separation of concerns. We want to completely separate the internals of Octokit from this application, so move any reference/call to Octokit into its own file to symbolize that.
|
Hey, @edmundmiller. Everything is ready to go, except for the filename lint rules. I can't find what these rules are, or where they come from. Could you point me in the right direction, and then I can fix that? |
…xtflow into feature/release-model
These linting rules are quite hidden: the culprit was this one: https://github.com/github/eslint-plugin-github/blob/main/lib/configs/recommended.js#L21 |
|
You can read the rate limit in the response header under |
|
|
@MatthiasZepper where have you been hiding that??? But I was actually talking about in this code base to see how many we're using and then do a back of the napkin calculation. |
Not sure why? 😕 There are exactly |
To make sure we don't introduce any regressions because it's been a huge pain during hackathons and for Sarek, and we're about to push it to the limit with the scatter CI in methylseq that I hope to add to the template. |
In plain sight, obviously. Bragged about it a lot - so often that I already felt bad about it. Regarding scatter: Just mind that the billable minute are calculated per job level. As far as I know, the nf-core organization doesn't have to pay for the Actions' runtime, but should GitHub ever change its pricing model, we need to be careful as separating everything into nano-jobs will have a great impact then. |
Regressions in what, exactly? You're worried that the action is going to today use, say 5 API calls, and then we might break it by introducing a bug that uses, say 10 API calls?
I get that, but that's during unit testing of pipelines, not of this action, right? So in that case, trying to find out the timeouts beforehand would happen exactly as we've talked about in #19, and we can add timeout and cooldown abilities to OctokitWrapper easily now. |
Yeah I think we're going to have to cross that bridge if they change the pricing model. I'm not following though, you're saying if they charge per job? I think the math would come out pretty close, the tests are taking 1-2 minutes a piece. The way it's broken up, we could shard them into 4 sets as well, instead of 20 jobs. That could be a better idea anyway because we'll start saturating the free runners quickly. |
Exactly! Anyway it was just a wish list item, I made #30 if we ever get time/motivation to follow up so good to merge with me @MillironX ! |
|
Firstly, sorry @MillironX for hijacking the discussion on your PR. I am neutral and will not submit a review on this PR, since I not even remotely understand all the consideration that went into it.
I was referring to this section from the HowTo article on Token rotation from Shopify:
|
Background
This is phase two of my four-part plan to fix the gripes with this action
This PR
Separate API calls from install logic
Merry Christmas, nf-core team! Here is a major overhaul to the action code, which will allow us to add mocks, create unit tests, and potentially even substitute caches or external APIs when the GitHub API hits a rate limit. I might want to add a few more tests before merging, but would love feedback now.