Extend tests for Azure.create_pull_request(..., reviewers)

[trejjam]

This PR adds support for reviewers in the create_pr.

Azure doc: https://docs.microsoft.com/en-us/rest/api/azure/devops/git/pull%20requests/create?view=azure-devops-rest-6.0#request-body

[jurre]

Thanks for the contribution!

I'm going to trigger CI, but I notice some things that'll definitely hit the linter, and I expect most tests for this class will now fail.

Speaking of tests, it'd be great to add some test cases for this that show how it works, it wasn't immediately clear to be from the docs what structure we need to pass to reviewers, currently it'll pass an array of hashes (objects in JSON) with an id key, is that right?

[trejjam]

Executing tests on Windows 10 was not really smooth ... But I manage it (with a few fixes)

I did add tests for creating the Azure PR

[jurre]

Executing tests on Windows 10 was not really smooth

Yeah I'd recommend using the docker dev environment to run tests, there are quite a few things that need to be set up in order to run it outside of docker. I personally always run inside the docker env

I did add tests for creating the Azure PR

Thanks!

[jeffwidman]

@trejjam for this one, two requests:

1. Can you rebase to fix merge conflicts? Also squash any fixup commits so every commit is a self-contained logical changeset.
2. Can you split out the Windows-compatibility fixes and the .gitattributes changes as separate PRs? They're entirely unrelated to the logic change for adding Azure reviewers... To be clear, I'm not opposed to them, but they shouldn't be shoehorned into this PR.

[trejjam]

.gitattribute PR: #5731

[jeffwidman]

IMO we need to do one of two things:

1. decide we want to support reviewers as a top-level arg across all the clients that support it... planning to eventually also add plumbing support for it in github/gitlab/bitbucket even if they're no-ops for now. But even in this case, we also have to decide if at the client layer we make it an optional arg or a required arg.
2. Not let this leak from Azure upwards... not even sure this is possible with the current code layout. But if we're only doing it for Azure, I'd rather not add an arg globally.

At first glance I lean towards making it a global optional arg, and then also optional at the individual client layer... reviewers really aren't a required thing to open PR's, but they are convenient and likely supported across pretty much all platforms...

I'm going to noodle on this API design a little and reach out to a teammate to get a second opinion.

In the meantime, no need to modify this PR, just leave as-is for now.

[jeffwidman]

Note to self: We could make this an optional arg instead of required here... depends whether we make reviewers a global required arg vs a global optional or local to azure arg.

I want to noodle on this API a little...

[jeffwidman]

Thanks for the fast rebase and splitting out as separate commits. Give me a little bit of time to think on the API design.

[jeffwidman]

After noodling on this more, "requested reviewers" is a pretty ubiquitous concept... I checked GitHub, BitBucket, GitLab, and they all support it. So I'm 👍 on the path this PR took of adding reviewers as an optional arg at the top level... with the expectation that over time, other folks will probably submit PR's to plumb this through for other platforms...

@trejjam can you fix merge conflicts?

The code in this PR currently all looks good to me, so unless another maintainer feels differently, once those conflicts are fixed this should be good to merge.

[trejjam]

Duplicate #5988

[trejjam]

In the meantime, someone else implemented the feature...

So this simply becomes tests extension...

[trejjam]

And the tests are failing, and I do not have the will to fix them

[jeffwidman]

Oh man, I'm so sorry! I completely forgot about #5988, my apologies!

And when looking at that PR, we should have pursued this one first... but we just weren't caught up on our backlog so completely overlooked it.

Again, my sincere apologies.