Get author from blame #60

sidsprasad · 2024-01-15T02:04:22Z

Modified Dockerfile to use alpine base
Added the new flag
Added git command to mark directory as safe for running git cmds
parallelly get the assignees for the newly created issues
edit the newly created issues by adding assignees after they've been created.
Upgraded libs

- Modified Dockerfile to use alpine base - Added the new flag - Added git command to mark directory as safe for running git cmds - Upgraded libs

sidsprasad · 2024-01-15T03:13:59Z

@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.

ribtoks

Sorry for the wait, just got to this. There're some changes needed

ribtoks · 2024-01-15T18:30:28Z

README.md

@@ -60,6 +60,7 @@ You can use this action together with [parent issue updater](https://github.com/
 | `CLOSE_LIMIT` | Upper cap on the number of issues to close (defaults to `0` - unlimited) |
 | `COMMENT_ON_ISSUES` | Leave a comment in which commit the issue was closed (defaults to `0` - do not comment) |
 | `CONCURRENCY` | How many files to process in parallel (defaults to `128`) |
+| `AUTHOR_FROM_BLAME` | Get the author of the todo via git blame if not already specified in the comment (defaults to '0' - do not use) |


AUTHOR_FROM_BLAME here and in action.yml it's ASSIGN_FROM_BLAME.

ribtoks · 2024-01-15T18:31:36Z

main.go

@@ -270,7 +273,7 @@ func (s *service) createProjectCard(issue *github.Issue) {
 log.Printf("Created a project card. issue=%v card=%v", issue.GetID(), card.GetID())
 }

-func (s *service) openNewIssues(issueMap map[string]*github.Issue, comments []*tdglib.ToDoComment) {
+func (s *service) openNewIssues(issueMap map[string]*github.Issue, comments []*tdglib.ToDoComment, newIssueMap *map[string]*github.Issue) {


Let's move newIssueMap to be a field of service. Then there will be less juggling around of these pointers.

P.S. There's no need to pass a pointer of map somewhere in Go, because maps are already reference types (like "automatic pointers").

ribtoks · 2024-01-15T18:32:35Z

main.go

@@ -325,6 +331,57 @@ func (s *service) openNewIssues(issueMap map[string]*github.Issue, comments []*t
 log.Printf("Created new issues. count=%v", count)
 }

+func (s *service) assignNewIssues(issueMap map[string]*github.Issue, assigneeMap *map[string]string) {


Let's move assigneeMap to become a member of service

main.go

ribtoks · 2024-01-15T18:37:25Z

main.go

+ }
+}
+
+func (s *service) getAssigneeFromCommitHash(commitHash string, title string, assigneeMap *map[string]string, successCount *int, assigneeMapMux sync.Locker, wg *sync.WaitGroup) {


Usually method getXYZ() returns something. This method does not return anything. I propose to rename it to retrieveCommitAssignees() or something along these lines.

There's no need to store successCount as the size of assigneeMap is equal to successCount

Regarding point 2, successCount is not incremented if there is an error getting the commit author while calling the github API. So, not always the size of assigneeMap.

Okay, I understand what you mean now 😛
Fixing this.

ribtoks · 2024-01-15T18:48:56Z

main.go


 log.Printf("Waiting for issues management to finish")
 svc.wg.Wait()

+ if env.assignFromBlame && len(newIssuesMap) > 0 {
+ log.Printf("Adding assignees to newly created issues.")


nit: should move this logline to assignNewIssues() for clarity

main.go

ribtoks · 2024-01-15T18:51:32Z

main.go

+ _, ok := issueMap[c.Title]
+ if !ok {


nit: these 2 lines can be written more "Go-idiomatically" as:

if _, ok := issueMap[c.Title]; !ok {

ribtoks · 2024-01-15T18:54:13Z

main.go

+func (s *service) getAssigneesForNewIssues(issueMap map[string]*github.Issue, comments []*tdglib.ToDoComment, assigneeMap *map[string]string) {
+ defer s.wg.Done()
+
+ var wg sync.WaitGroup


What exactly is the reason we need a new wait group and cannot reuse the one from the service? We're still waiting for that wait group in the main() before we return from here.

I did this because I wanted to output the number of issues for which we successfully got assignees. So, for that, we want all the threads to have completed. If we use the other existing wg, then we would have to get this info only after the wait command for that, but that is outside this function and the code would be a bit messy.

ribtoks · 2024-01-15T18:56:02Z

main.go

+ log.Printf("Error while getting commit from commit hash. err=%v", err)
+ } else {
+ assigneeMapMux.Lock()
+ defer assigneeMapMux.Unlock()


For short code blocks you don't need defer to unlock the mutex. It's more fail-safe, since if you did that in a loop, it would be a deadlock with defer

sidsprasad · 2024-01-16T03:41:15Z

@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 · 2024-01-16T04:40:30Z

@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 · 2024-01-16T04:54:14Z

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

sidsprasad · 2024-01-16T05:01:07Z

@ribtoks , made the calls to commitAPI linear 👍

sidsprasad · 2024-01-16T06:31:35Z

@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.

ribtoks

1 or 2 safeguarding comments + few very cosmetic beauty improvements (for you to consider).

P.S. Please do not force-push. In such way I can only review the diff from your latest changes, instead of having to review the whole thing again.

ribtoks · 2024-01-16T15:08:29Z

action.yml

@@ -56,6 +56,9 @@ inputs:
 COMMENT_ON_ISSUES:
 description: "Create commit reference in the comments before closing the issue"
 default: "0"
+ ASSIGN_FROM_BLAME:
+ description: "Assign issue to the author of the comment via git blame"


nit: Actually now this is a misleading documentation because we assign from GitHub API. I think we should specify this, as this is a not obvious feature. Also then name of the parameter is somewhat mismatched with reality.

main.go

ribtoks · 2024-01-16T15:12:23Z

main.go

+ client: github.NewClient(tc),
+ env: env,
+ newIssuesMap: make(map[string]*github.Issue),
+ assigneeMap: make(map[string]string),


nit: In general would be nice to use a more descriptive name, like commitToAssigneeMap OR have a comment that explains that. Because we will forget this in 1 month. Also applicable to commitAuthorCache (which is much better, just might become commitToAuthor Map/Cache)

ribtoks · 2024-01-16T15:13:51Z

main.go

+ }
+
+ command := "git"
+ args := []string{"config", "--global", "--add", "safe.directory", root}


Question. I thought this is a responsibility of tdg? Why do we need this here?

UPD. Is it because we're using tdglib and this was done by tdg (executable) before?

You are right. I can't rememebr exactly why, but before I had thought it wouldn't fit right in tdg. But now I think it would fit better in tdg. But you're saying that tdg is already doing this? I don't think so. Will create an MR for it on Gitlab.

ribtoks · 2024-01-16T15:16:17Z

main.go

@@ -325,6 +333,55 @@ func (s *service) openNewIssues(issueMap map[string]*github.Issue, comments []*t
 log.Printf("Created new issues. count=%v", count)
 }

+func (s *service) assignNewIssues() {
+ log.Printf("Adding assignees to newly created issues.")


nit:

It's a print F (format) without any format modifiers %x inside. Either something is missing from inside or it can become a Println()

Might be useful to actually add something there. e.g. "Adding assignees to %v newly created issues", len(s.assigneeMap)

ribtoks · 2024-01-16T15:17:20Z

main.go

+ req := &github.IssueRequest{
+ Assignees: &[]string{assignee},
+ }
+ _, _, err := s.client.Issues.Edit(s.ctx, s.env.owner, s.env.repo, issueNumber, req)


nit: just for beauty, this can also become if _, _, err := ....Edit(); err != nil { - merged with next line.

sidsprasad · 2024-01-17T08:08:36Z

@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 · 2024-01-17T08:28:39Z

@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 · 2024-01-17T08:49:12Z

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

sidsprasad · 2024-01-17T09:15:18Z

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

ribtoks · 2024-01-17T09:24:50Z

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

sidsprasad · 2024-01-17T09:26:25Z

@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 😄

sidsprasad added 4 commits January 15, 2024 11:07

Add AUTHOR_FROM_BLAME flag

41c0987

- Modified Dockerfile to use alpine base - Added the new flag - Added git command to mark directory as safe for running git cmds - Upgraded libs

Add assignees to git issue

4191d4b

Remove trailing . and / from root path in git cmd

cfb5b92

correct commit API, add committerEmail to body

1bab808

sidsprasad force-pushed the get_author_from_blame branch 4 times, most recently from 8c6b137 to 5e828ae Compare January 15, 2024 02:37

Paralelly get assignees and assign to issues later

4efa5fe

sidsprasad force-pushed the get_author_from_blame branch from 5e828ae to 4efa5fe Compare January 15, 2024 02:52

Use new waitGroup for getting assignees go routine

0fd3299

ribtoks requested changes Jan 15, 2024

View reviewed changes

Made changes based on review

0ee814a

sidsprasad force-pushed the get_author_from_blame branch 2 times, most recently from fff365b to 0ee814a Compare January 16, 2024 04:40

Make calls to commitAPI linear instead of parallel

7e15dbf

sidsprasad marked this pull request as draft January 16, 2024 05:49

sidsprasad force-pushed the get_author_from_blame branch from 1c42304 to b71306e Compare January 16, 2024 05:54

Cache the commits retrieved

a77e256

sidsprasad force-pushed the get_author_from_blame branch from b71306e to a77e256 Compare January 16, 2024 06:29

sidsprasad marked this pull request as ready for review January 16, 2024 06:30

ribtoks requested changes Jan 16, 2024

View reviewed changes

Make changes from second review

c823420

sidsprasad added 2 commits January 17, 2024 18:10

Upgrade tdg to v0.0.4 from v0.0.3

d41e1f7

Remove code to mark root dir safe for git

e0c3026

ribtoks approved these changes Jan 17, 2024

View reviewed changes

ribtoks merged commit 3482957 into ribtoks:master Jan 17, 2024

ribtoks mentioned this pull request Jan 17, 2024

[Feature] Allow assigning Author on PR Branch #32

Closed

sidsprasad deleted the get_author_from_blame branch January 17, 2024 09:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get author from blame #60

Get author from blame #60

sidsprasad commented Jan 15, 2024

sidsprasad commented Jan 15, 2024 •

edited

Loading

ribtoks left a comment

ribtoks Jan 15, 2024

ribtoks Jan 15, 2024

ribtoks Jan 15, 2024

ribtoks Jan 15, 2024

sidsprasad Jan 16, 2024

sidsprasad Jan 16, 2024

ribtoks Jan 15, 2024

ribtoks Jan 15, 2024

ribtoks Jan 15, 2024

sidsprasad Jan 16, 2024

ribtoks Jan 15, 2024

sidsprasad commented Jan 16, 2024 •

edited

Loading

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

ribtoks left a comment

ribtoks Jan 16, 2024

ribtoks Jan 16, 2024

ribtoks Jan 16, 2024 •

edited

Loading

sidsprasad Jan 17, 2024

ribtoks Jan 16, 2024

ribtoks Jan 16, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

ribtoks commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

Get author from blame #60

Get author from blame #60

Conversation

sidsprasad commented Jan 15, 2024

sidsprasad commented Jan 15, 2024 • edited Loading

ribtoks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidsprasad commented Jan 16, 2024 • edited Loading

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

sidsprasad commented Jan 16, 2024

ribtoks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ribtoks Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

ribtoks commented Jan 17, 2024

sidsprasad commented Jan 17, 2024

sidsprasad commented Jan 15, 2024 •

edited

Loading

sidsprasad commented Jan 16, 2024 •

edited

Loading

ribtoks Jan 16, 2024 •

edited

Loading