-
Notifications
You must be signed in to change notification settings - Fork 183
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
Make the user agent configurable for link checker scripts #1302
Conversation
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
3495b44
to
8467d65
Compare
The following pipelines have been queued for testing: |
@@ -8,7 +8,7 @@ parameters: | |||
Urls: '(Get-ChildItem -Path ./ -Recurse -Include *.md)' | |||
BranchReplaceRegex: "^(${env:SYSTEM_PULLREQUEST_SOURCEREPOSITORYURI}.*/(?:blob|tree)/)master(/.*)$" | |||
BranchReplacementName: "${env:SYSTEM_PULLREQUEST_SOURCECOMMITID}" | |||
|
|||
UserAgent: ${env:USERAGENT} |
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.
Should we just default this to empty? Or do you expect that we would set this as a pipeline variable?
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 plan to set it through pipeline variable as it does not affect all pipelines. Just want to limit use the custom userAgent setting.
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.
Some links prefer specific userAgent. We currently do not have the full picture of all scenarios, so I prefer to use pipeline UI variable which is easy to test and modify.
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.
If we can get the default of Chrome to work in our usages (I'd test it in the azure-sdk link checking pipeline), then lets just remove this in the yml. If we decide to keep this we should make the variable name more specific something like VERIFY_LINK_USERAGENT, so that we don't accidently clash with others.
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.
Chrome PSUserAgent does not work for the twitter link, I have to switch to "Chrome/87.0.4280.88" to invoke twitter link
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.
Working one:
Invoke-WebRequest -Method GET -URI "https://www.twitter.com/AzureSDK" -UserAgent "Chrome/87.0.4280.88"
Default one not working:
Invoke-WebRequest -Method GET -URI "https://twitter.com/AzureSDK" -UserAgent "[Microsoft.PowerShell.Commands.PSUserAgent]::Chrome"
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.
Maybe just default to the newer Chrome version in the script as well.
eng/common/scripts/Verify-Links.ps1
Outdated
[bool] $checkLinkGuidance = $false | ||
[bool] $checkLinkGuidance = $false, | ||
# UserAgent to be configured for web request. Default to PSUserAgent. | ||
[string] $userAgent = "Microsoft.PowerShell.Commands.PSUserAgent" |
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 wonder if we should default it to the most common browser useragent string so we can seem like a browser when checking.
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.
We can choose either IE or Chrome here. I can set to Chrome if no other preference.
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 chrome user agent string would be fine.
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
No description provided.