-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(azure-devops): add check for test webhook URL #2809
Conversation
Sorry @nitrocode - Not had time over the holidays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you!
@@ -43,6 +43,9 @@ const bitbucketCloudRequestIDHeader = "X-Request-UUID" | |||
const bitbucketServerRequestIDHeader = "X-Request-ID" | |||
const bitbucketServerSignatureHeader = "X-Hub-Signature" | |||
|
|||
// The URL used for Azure DevOps test webhooks | |||
const azuredevopsTestURL = "https://fabrikam.visualstudio.com/DefaultCollection/_apis/git/repositories/4bc14d40-c903-45e2-872e-0462c7748079" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come this url has to be hardcoded?
Shouldn't this value be in the test instead?
Who has access to use this url for testing purposes?
Couldn't this be mocked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR is to detect and ignore the test webhooks that come from Azure DevOps. The test requests for the 3 webhooks that Atlantis uses are in the PR description. (The tests aren't configurable, they just send the same requests every time)
The URL is a common property that exists in all three requests so we're using it to say: "If the request says it's coming from this URL: Ignore the request"
No access is needed to this URL, we're simply using it to identify these test requests.
Does that sound okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think we'll ever have to configure these urls?
Would you add some additional comments in the code to add this explaination in case people have this question in the future?
@@ -43,6 +43,9 @@ const bitbucketCloudRequestIDHeader = "X-Request-UUID" | |||
const bitbucketServerRequestIDHeader = "X-Request-ID" | |||
const bitbucketServerSignatureHeader = "X-Hub-Signature" | |||
|
|||
// The URL used for Azure DevOps test webhooks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Has this PR been tested in your atlantis deployment? Were you able to verify that this change worked as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll build it and run through some tests - hold off until I've confirmed that's sorted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! Once you confirm then we can merge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SSKLCP friendly ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SSKLCP friendly ping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SSKLCP friendly ping
Co-authored-by: Ken Kaizu <k.kaizu38@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @SSKLCP ! |
* Adding check for Azure DevOps Test Webhook URL so they aren't processed like real events * feat: customize vcs comment command executable name * Altering case on isAzureDevOpsTestRepoURL function * Altering case on URL parameter * Altering checks to ensure all Azure DevOps Ignore tests pass * Extending setup method to support Azure DevOps * Fixing issue from merge conflict error * gofmt * Adding null checks to Test URL checks * moving azuredevopsTestUrl to const * Changing azuredevopsTestUrl to azuredevopsTestURL * Cleaning up return statement Co-authored-by: Ken Kaizu <k.kaizu38@gmail.com> --------- Co-authored-by: Ken Kaizu <k.kaizu38@gmail.com> Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
* Adding check for Azure DevOps Test Webhook URL so they aren't processed like real events * feat: customize vcs comment command executable name * Altering case on isAzureDevOpsTestRepoURL function * Altering case on URL parameter * Altering checks to ensure all Azure DevOps Ignore tests pass * Extending setup method to support Azure DevOps * Fixing issue from merge conflict error * gofmt * Adding null checks to Test URL checks * moving azuredevopsTestUrl to const * Changing azuredevopsTestUrl to azuredevopsTestURL * Cleaning up return statement Co-authored-by: Ken Kaizu <k.kaizu38@gmail.com> --------- Co-authored-by: Ken Kaizu <k.kaizu38@gmail.com> Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>
…so they aren't processed like real events
what
When setting up the webhooks in Azure DevOps you get an option to test them. Running this test fails because it points you to a repo that Atlantis has no access to and can fail ungracefully.
Adding in a check for the test webhook requests and not process them like normal requests, Returns a status 200 as the web request is working.
The check relies on the Repo URL equalling: https://fabrikam.visualstudio.com/DefaultCollection/_apis/git/repositories/4bc14d40-c903-45e2-872e-0462c7748079
Which is consistent across all test webhooks
why
Confusing when the webhooks are setup correctly and it shows a fail for the test.
We don't like ungraceful failures.
help
This is a first draft of what the solution could be. It feels a little bit janky to me and wasn't sure if you have a better idea of how to implement it?
The URL seemed like the most unique similarity between all the requests - Annoyingly they didn't all have a "this is a test" property.
The Commented event json is a different format to the new/updated json so I've had to put in the check twice depending on its route rather than one high level check. Again, not sure if there's a better way.
Here's all the web request bodies for the test webhooks in case you want to take a look:
Pull Request Commented On
Pull Request Created
Pull Request Updated
references