search

gruntwork-io / git-xargs

git-xargs is a command-line tool (CLI) for making updates across multiple Github repositories with a single command.
https://blog.gruntwork.io/introducing-git-xargs-an-open-source-tool-to-update-multiple-github-repos-753f9f3675ec
Apache License 2.0
935 stars 62 forks source link

feat: Add --draft flag for draft pull requests #49

Closed ryanwholey closed 2 years ago

ryanwholey commented 2 years ago

Hey folks, thanks for the neat tool!

My team is interested in using the GitHub drafts feature so I've implemented a --draft flag here.

I played around with a few tests but I didn't find one that felt like it provided meaningful coverage. What we want I think is a test where we check that the PR submitted to client.PullRequests.Create has the has Draft: true, which would require some storage structure on the mock used in perhaps another test like TestProcessRepo. This did seem like a lot for a flag, so wanted to see what you thought before adding it. Open to any suggestions for testing approaches!

Also looks like my editor removed a couple extra spaces, please let me know if you'd like them reverted.

closes #47

ryanwholey commented 2 years ago

Hey @brikis98 @zackproser! Any interest in the changes proposed here? Thanks!

zackproser commented 2 years ago

Hi @ryanwholey,

Thanks so much for contributing to git-xargs and apologies for the delay in getting back to you.

First off, this looks like a great start and I do believe we'd be interested in this functionality. There are a couple of things we'd probably want to include before this is ready to merge, however:

  1. Error handling for when draft pull requests are not supported. For example, I have a personal Github org that I use for testing. It is currently comprised of only private repos, which currently do not support draft PRs according to Github's docs - and, when testing this locally against my test org, if I pass the --draft flag and the --loglevel DEBUG flag, I can see HTTP 422 errors being returned as a result. We should probably handle this error specifically
  2. In order for operators to be notified of any pull requests that could not be opened as drafts in the final run report, we'll need to wire up a new event type in stats/stats.go and add it to the slice of all events in that same file. This way, when someone attempts to open draft pull requests on a repo that does not support them, they'll get a detailed breakdown of exactly which repos had this issue, so it will be easier to troubleshoot.
  3. The additional test coverage, as you suggested. This would be ideal to have in place before merging. However, if it's not something you have the interest or bandwidth to implement, we could do it on our end, but this may take a bit longer.
ryanwholey commented 2 years ago

@zackproser all good, thank you for the direction! I'll dive in and update soon

ryanwholey commented 2 years ago

Hi @zackproser, finally getting back around to this!

I've added handling for 422's under the event type RepoNotCompatibleWithPullConfig. Looks like the GitHub client doesn't export more specified errors here for something like draft incompatibility. Let me know if you think we need some more targeted copy shown to the user.

Regarding updating the testing framework here, I don't believe I'll have time to get to this any time soon. Please let me know if you'd like to see some other form of validation here with regard to these changes.

zackproser commented 2 years ago

Hi @zackproser, finally getting back around to this!

I've added handling for 422's under the event type RepoNotCompatibleWithPullConfig. Looks like the GitHub client doesn't export more specified errors here for something like draft incompatibility. Let me know if you think we need some more targeted copy shown to the user.

Regarding updating the testing framework here, I don't believe I'll have time to get to this any time soon. Please let me know if you'd like to see some other form of validation here with regard to these changes.

Hi @ryanwholey,

Awesome - thanks again for implementing this! I was able to get draft PRs opened using your branch against my test org, so that's great.

I have a couple of nits and thoughts around how to best describe the draft PR activity in the final run report that gets rendered for operators.

I'm going to keep playing around with this and leave some feedback on your PR. Understood on time constraints, so I will probably push some commits there, too.

zackproser commented 2 years ago

I opened https://github.com/gruntwork-io/git-xargs/issues/61 to track the implementation of additional test coverage for draft PRs.

zackproser commented 2 years ago

Thanks for the review! Going to merge this in now, as tests also passed locally.

zackproser commented 2 years ago

Thanks so much @ryanwholey ! https://github.com/gruntwork-io/git-xargs/releases/tag/v0.0.13