ribtoks / tdg-github-action

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

Add AUTHOR_FROM_BLAME flag #56

Closed sidsprasad closed 9 months ago

sidsprasad commented 9 months ago
sidsprasad commented 9 months ago

@ribtoks here is a preview of my changes. Got everything working so far, now I only need to make it assign the issue to the right person on GitHub. Once I sort that, will mark this as ready. 😄

sidsprasad commented 9 months ago

@ribtoks So, I played around a bit, and it looks like you have to pass the github username only to the IssueRequest API. If the user hasn't added an email to their local git settings, then the username is by default set to either ID+USERNAME@users.noreply.github.com or USERNAME@users.noreply.github.com for github repos as detailed here. In these cases, we can get the username from the email. In cases where the user has added their email, we would have to get their username using the github APIs. BUT, this can only happen if a user's email is made public on their profile. By default it is set to private, sadly. So, I don't know if there is a clean way to assign the issues to a particular user on github in this last case. 😢 😞

sidsprasad commented 9 months ago

@ribtoks I suppose one way to handle this would be to have a config file that the user can edit specifying a map between users and their git emails. And pass this config to the script via the action XML. And we can also handle the base case of no registered email by extracting the username from the standard email formats I had mentioned above in my previous comment. And if no valid username is found, then we fail the pipeline.

ribtoks commented 9 months ago

@sidsprasad I highly doubt that anybody will want to create such a config mapping file to use this GitHub Action. It's much simpler to use tdg comment markup that already supports author=xyz tag in the comment. Also I don't think we can count that users made their email public.

If we're creating an issue and specifying an author from the email and it's incorrect (e.g. it's already incorrect even for my account), the IssueRequest API will fail and this is not what we want. We want to create the issue for sure and only maybe assign to the author. Just to keep in mind.

Other approach could be to assign issues based on CODEOWNERS file(s), that is used for PRs, but this is not exactly what you're after.

ribtoks commented 9 months ago

@sidsprasad Just to reiterate, I do not think that adding a mapping file is a good way to move forward for this project. I think it creates an additional maintenance burden to setup the action and to maintain this mapping (for user changes in username and/or email)

sidsprasad commented 9 months ago

@ribtoks thanks for getting back quickly, as always. 😄

Also I don't think we can count that users made their email public.

I agree. Most people I know have their emails set to private.

I highly doubt that anybody will want to create such a config mapping file to use this GitHub Action. It's much simpler to use tdg comment markup that already supports author=xyz tag in the comment.

I think for sufficiently large projects (such as the one I want to use tdg for), it would be worth the effort. For other projects, if they don't want to set it up this way, they would use it without this option.

If we're creating an issue and specifying an author from the email and it's incorrect (e.g. it's already incorrect even for my account), the IssueRequest API will fail and this is not what we want. We want to create the issue for sure and only maybe assign to the author. Just to keep in mind.

I don't completely agree with this. For example, in my use case for this, we have about 200 TODOs and after speaking with devs, I realized that a lot of them have been forgotten about. And the main goal of using a tool such as tdg was to manage this technical debt better. If issues are created and not assigned to people, then in large projects with many issues already, these new todo issues could just be lost. So, it would work better for us if the pipeline failed and flagged to us that issues were not being assigned properly.

So, it's all a matter of perspective (As with everything lol).

Hope my comment here makes sense, and you'd reconsider this method. No worries if not too. Will remove this mapping part from this PR and just go ahead with the remainder so we can get the email at least from the blame and have it on the issue body. And then I could write something on our end to assign the issues accordingly after the issue was created by tdg with the email tagged in the body.

Thanks a lot for your quick responses. Look forward to hearing back 😃

ribtoks commented 9 months ago

@sidsprasad In cases of large influx of TODOs (e.g. you run tdg for the first time on the project), you need to triage all the issues anyways first. On GitHub for that I personally setup a Project automation that adds new issues with a specified label (that's why tdg-github-action has the label configuration) to the triage column and in that project the triage happens.

Regarding username to email map, I cannot reconsider at this time. I think this could be a maintenance overkill.

Now, regarding possible solutions. I just asked ChatGPT and it said that you can get username from commits API. And indeed, after short search, I found that commit API returns you a json that has .author.login ("jq syntax") field that contains the GitHub username. I think this API could be an overkill too and I assume there're some other ways too. But I wanted to suggest you to go over the GitHub REST API to examine if there're some easy ways to achieve the same. If not, then commit API could be the way. This can be done in the tdg-github-action in parallel process to creating the issues. Once you've got the mappings, you would be able to assign issues to authors later.

UPD. In the light of this: if you will want to go down this path (GitHub API), might be needed to extend tdg (on GitLab) to return commit hash in addition to email. This should be available from the git blame output that you're parsing anyways.

sidsprasad commented 9 months ago

@ribtoks thanks for the info. The commit API seems good to me. I have created a new MR on gitlab here: https://gitlab.com/ribtoks/tdg/-/merge_requests/9