Skip to content

Conversation

@cjboden
Copy link

@cjboden cjboden commented May 11, 2021

Fixes #95. Pull requests can be retrieved by ID using the following:

Get-ADORepository -Project MyProject -RepositoryID RepoID -PullRequestID 123

@cjboden cjboden marked this pull request as ready for review May 11, 2021 17:20
@StartAutomating
Copy link
Owner

The change looks good (and it looks like you get how subtle adding a new endpoint can be).

Tests most likely failed due to the identity of the runner. I've re-run and hopefully it will be able to use the SystemAccessToken correctly as "me".

@StartAutomating
Copy link
Owner

@cjboden It looks like the tests have a few places where they fail when getting a PR from another repo.

It also looks like I'm not able to force the identity to switch over at this point.

While this isn't core to your PR, could you please alter the tests to skip tests in CI/CD when testing a fork?

This link will have details about predefined variables in Azure DevOps. $env:SYSTEM_PULLREQUEST_ISFORK will probably have this context.

Additionally, if you wanted to go for bonus points:

  1. I'm sure there are more Repository-releated Endpoints you might want Get-ADORepository to expose. Feel free to add more.
  2. Pull Requests could probably use a formatter, if you wanted to write one.

@cjboden
Copy link
Author

cjboden commented May 11, 2021

@StartAutomating It was a lot easier to add that in than I thought it would be. I kept thinking I'd missed something but my manual tests seemed to come out just fine. Your solutions are much more robust than what I had built before I found PSDevOps this morning.

I'll take a look at the test skipping and push up a change with that in it. Alternatively, would you like me to push these changes to a branch within this repository and resubmit the PR from that so the tests can run? I did it this way just because I wasn't sure how you wanted external contributions handled, since it looks like every contribution was yours.

No other endpoints I can think of that I need/want at the moment, but if I find any I'll be sure to contribute!

@cjboden
Copy link
Author

cjboden commented May 12, 2021

@StartAutomating Do you believe it might work to have this as part of the pipeline? I wonder if this change would work:

  - stage: TestPowerShellCrossPlatform
    displayName: Test
    condition: eq(variables['System.PullRequest.IsFork'], false))

I'm no pipeline guru, but this should skip the test stage altogether if from a fork. What I don't know, though, is if that affects the succeeded() condition for the Update stage.

@StartAutomating
Copy link
Owner

@StartAutomating Do you believe it might work to have this as part of the pipeline? I wonder if this change would work:

  - stage: TestPowerShellCrossPlatform
    displayName: Test
    condition: eq(variables['System.PullRequest.IsFork'], false))

I'm no pipeline guru, but this should skip the test stage altogether if from a fork. What I don't know, though, is if that affects the succeeded() condition for the Update stage.

This is a cool trick!

That stated, I don't want to skip all tests, just ones that fail under these circumstances.

Any build variable also exists as an environment variable (just capitalize and replace . with _).

Thus I'm thinking that the right thing to do is to use $ENV:SYSTEM_PULLREQUEST_ISFORK -eq 'true' to skip tests when on a fork.

A number of tests use -WhatIf to avoid actually hitting the server, or read a public project.

Your solutions are much more robust than what I had built before I found PSDevOps this morning.

Why thank you! I'm especially proud of some of the RESTful wrapping tricks I've come up with in PSDevOps. As you found out, it's especially easy to add endpoints with this whole approach. Feel free to reach out if you'd like to discuss this more (I'm often on the PowerShell Discord). I'm also happy to invite you to the project's Azure DevOps instance.

Alternatively, would you like me to push these changes to a branch within this repository and resubmit the PR from that so the tests can run?

You're welcome to do this as well. This would be less effort for you, though it doesn't make the build more stable in the long run.

@cjboden
Copy link
Author

cjboden commented May 12, 2021

I think I've got something that should work. I store a value $IsFork based on that env var and then add a -Skip:$IsFork to any of the context/is blocks that need to be skipped based off of the tests the output the pipeline showed as failed.

@cjboden
Copy link
Author

cjboden commented May 12, 2021

Alright well it is taking a couple of tries... The Pester documentation says that Context has a -Skip parameter but it appears that it really doesn't.

Edit: Looks like somebody copy/pasted something in their docs wrong. The description for -Skip actually shows it is defined as $Focus, which has a different purpose than skip...

@StartAutomating
Copy link
Owner

Thanks for going the extra mile!

Merging the PR. Feel free to send more.

@StartAutomating StartAutomating merged commit 737eab6 into StartAutomating:master May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Retrieve Pull Request by ID

2 participants