search

ribtoks / tdg-github-action

GitHub action for project management using TODO comments
MIT License
58 stars 5 forks source link

Get author from blame #60

Closed sidsprasad closed 9 months ago

sidsprasad commented 9 months ago
sidsprasad commented 9 months ago

@ribtoks ready for review! As discussed, I created a routine to parallely get assignees for new issues from the commit hash. And then only after the new issues have been created, do we edit those issues by adding assignees.

sidsprasad commented 9 months ago

@ribtoks Thanks for the review. I have left some comments above and made the changes discussed and pushed EXCEPT for the Git API limits discussion and cache. I will look into them now, but pushed the other changes for you to review in the meantime 😄

sidsprasad commented 9 months ago

@ribtoks okay, so we can look at the rate limits in the RateLimits attribute of the github client. When the client is authorized, it usually has 1000 requests per hour. But if it is unauthorized, only 60 per hour.

In general, our code used to call the API n + C times, where n is the number of new/closed issues and C is the number of requests it would take to get all the issues (more than 1 if it is paginated).

With this change, it would call the API 3 * n + C times. +2n times because it gets the commit for each new issue and it edits the issues as well.

There is no issue with simultaneous requests as far as I can tell from the github documentation. There could be an issue with rates if the number of issues is more than one-third of the rate limit.

So, what I propose is that we could calculate the number of new issues and issues to close (n) before making any API calls at all, and then check if 3 * n < Rate Limit + T where T is some threshold we would set. If it's false, then we can cap the total number of isses it would create and only run for those. Then, the next time the pipeline runs, it would add the next batch of issues, and so on... What do you think about this?

Will start to code this logic while waiting for you to get back.

sidsprasad commented 9 months ago

Oh sorry, I might be wrong about the concurrent requests thing. Taking a look at that.

sidsprasad commented 9 months ago

@ribtoks , made the calls to commitAPI linear 👍

sidsprasad commented 9 months ago

@ribtoks Added cache as well. Ready for review. My earlier comment about limiting the number of API calls could be a future ticket, as this is something that would have had to be dealt with even outside of my change. Let me know what you think.

sidsprasad commented 9 months ago

@ribtoks I usually only force push to new commits after the one you had reviewed (Usually when I made a mistake and want to fix it in the same commit). I didn't realize this messes it up while reviewing. Okay, will not force push from now on.

Have made all the fixes mentioned above except the one related to marking the directory as safe, As mentioned, will create an MR for that in Gitlab.

sidsprasad commented 9 months ago

@ribtoks created the aforementioned MR over here: https://gitlab.com/ribtoks/tdg/-/merge_requests/11 Once that is merged, will remove that section of code from this PR.

sidsprasad commented 9 months ago

@ribtoks could you please create a new tag on tdg so that I can update it here and pull?

sidsprasad commented 9 months ago

@ribtoks Thank You! Just pushed all the changes here now. Ready for a final review.

ribtoks commented 9 months ago

@sidsprasad Amazing! Thank you for your contribution and navigating the maze of depedencies!

sidsprasad commented 9 months ago

@ribtoks and thank you so much for your patience! Learnt a lot from our interactions and your code reviews. Will start using this in one of my repositories now! Look forward to working with you in the future 😄