GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Change UserAgent App WindowsPowerShell -> PowerShell by markekraus · Pull Request #4914 · PowerShell/PowerShell

Conversation

@markekraus

@markekraus Mark Kraus (markekraus) commented Sep 25, 2017

Copy link
Copy Markdown
Contributor

fixes #4911

  • renames the App portion of the user agent to PowerShell
  • Adjusts tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
Change UserAgent App WindowsPowerShell -> PowerShell
ea9f5d8
@markekraus
Mark Kraus (markekraus) requested review from James Truher (JamesWTruher), Paul Higinbotham (PaulHigin) and Dan Travison (dantraMSFT) as code owners September 25, 2017 00:18
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-already-signed label Sep 25, 2017
@markekraus
[Feature] Run Feature tests
9313cdb
SteveL-MSFT
Steve Lee (SteveL-MSFT) approved these changes Sep 25, 2017
View reviewed changes

@SteveL-MSFT Steve Lee (SteveL-MSFT) left a comment

Copy link
Copy Markdown
Member

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.

Thanks!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@daxian-dbw Dongbo Wang (daxian-dbw) assigned Aditya Patwardhan (adityapatwardhan) Sep 25, 2017
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Sep 25, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
$jsonContent = $result.Output | ConvertFrom-Json
$jsonContent.headers.Host | Should Be $uri.Authority
$jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell"
$jsonContent.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

This match would work with WindowsPowerShell as well. This should be ^PowerShell.

In addition to that, it should be MatchExactly

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Collaborator

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.

User-Agent can be PowerShell/6.0.0-Beta.8 or PowerShell/6.0.0- so if we use MatchExactly the pattern should be "^PowerShell/\d+\.\d+\.\d+.*"
We use the pattern many times - it make sense to use a variable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 25, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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.

It was doing an imprecise partial match before.

It's actually more like Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.15063 ; en-US) WindowsPowerShell/6.0.0 and it will be more dynamic "soon"

we could pull the exact one through reflection in BeforeAll. Honestly, I'm not even sure this is a good test. Should Be $uri.Authority should be sufficient, IMO.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

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.

fixed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

# Validate response
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response
$result.Output.headers.Host | Should Be $Uri.Authority
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response
$result.Output.headers.Host | Should Be $uri.Authority
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response
$result.Output.headers.Host | Should Match $uri.Authority
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated

# Validate response
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response
$result.Output.url | Should Match $uri
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response
$result.Output.url | Should Match $uri
$result.Output.headers.'User-Agent' | Should Match "WindowsPowerShell"
$result.Output.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
$jsonContent = $result.Output | ConvertFrom-Json
$jsonContent.headers.Host | Should Be $uri.Authority
$jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell"
$jsonContent.headers.'User-Agent' | Should Match "PowerShell"

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Sep 25, 2017

Copy link
Copy Markdown
Member

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.

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
[feature] Address PR Feedback with more precise assertion
6ecbdcc
@markekraus

Mark Kraus (markekraus) commented Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

Test fails due to #4919
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov
Ilya (iSazonov) reviewed Sep 26, 2017
View reviewed changes

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

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.

Leave a comment

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
if ( test-path /etc/centos-release ) { $PendingCertificateTest = $true }

# Get the default UserAgent through reflection
$DefaultUserAgent = [Microsoft.PowerShell.Commands.PSUserAgent].GetProperty('UserAgent',@('Static','NonPublic')).GetValue($null,$null)

@iSazonov Ilya (iSazonov) Sep 26, 2017

Copy link
Copy Markdown
Collaborator

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.

It is dummy test: if C# code will be changed the test will be automatically passed. I believe we should use explicit desired pattern.
Also please move to BeforeAll.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

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.

That's not the purpose of the tests. They are just testing the response from the server to ensure that expected data sent is also returned. if we want to test the patterns in the UserAgent, that would be a separate set of tests for the PSUserAgent class.

I will add a separate describe block for PSUserAgent tests and a test for the App member (there will be other tests possibly for #4193). I will create a separate issue to add and track adding other tests for PSUserAgent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@msftgits Microsoft GitHub User (msftgits) removed the cla-already-signed label Sep 26, 2017
@markekraus
[feature] Address PR Feedback and add PSUserAgent Test
59e02b4
iSazonov
Ilya (iSazonov) reviewed Sep 26, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
Describe "PSUserAgent Tests" {
It "App Should Match ^PowerShell/\d+\.\d+\.\d+.*" {
$app = [Microsoft.PowerShell.Commands.PSUserAgent].GetProperty('App',@('Static','NonPublic')).GetValue($null,$null)
$app | Should Match '^PowerShell/\d+\.\d+\.\d+.*'

@iSazonov Ilya (iSazonov) Sep 26, 2017

Copy link
Copy Markdown
Collaborator

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.

Here I don't see the point of checking internal property - we are free to change (add/remove) internals in any time. We should test public API. Here "public API" is User-Agent which we send to a server and get it back. We should test the User-Agent format if we want to control the User-Agent. So it is good to use regex pattern. I think that check User-Agent 14 times it's redundant - we need to have one check for each web cmdlet.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

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.

Again, the point of most of those tests is not to test the User-Agent that is sent. Most of those assertions are being used to ensure meaningful data is returned in a response after a set of parameters has been used to make web requests. The only reason I updated them at all was because the change from WindowsPowerShell to PowerShell would have caused all these tests to false fail.

I will change the two tests that are explicitly about the User-Agent.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@iSazonov Ilya (iSazonov) Sep 26, 2017

Copy link
Copy Markdown
Collaborator

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.

As you mentioned $uri.Authority is sufficient. So I'd remove $DefaultUserAgent at all. I unlike it as you see 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

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'm ok with that. This entire file needs to be more deeply analyzed and cleaned up. Maybe after I'm done making radical changes to it every day 😆.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iSazonov reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Mark Kraus (markekraus) added 3 commits September 26, 2017 04:13
@markekraus
[feature] add tag to describe block
7198bec
@markekraus
[feature] Remove describe block Add pattern to User-Agent specific tests
671fc63
@markekraus
[feature] Address PF Feedback
ba056d9 
Remove all assertions for User-Agent that are not specifically about the User-Agent header.
iSazonov
Ilya (iSazonov) approved these changes Sep 26, 2017
View reviewed changes
JamesWTruher
James Truher (JamesWTruher) approved these changes Sep 26, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated
# Validate response content
$jsonContent = $result.Output.Content | ConvertFrom-Json
$jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell"
$jsonContent.headers.'User-Agent' | Should Match '(?<!Windows)PowerShell\/\d+\.\d+\.\d+.*'

@JamesWTruher James Truher (JamesWTruher) Sep 26, 2017

Copy link
Copy Markdown
Collaborator

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 think you might use MatchExactly which does a case sensitive match (-cmatch). Especially since you're explicitly not matching WindowsPowerShell

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markekraus Mark Kraus (markekraus) Sep 26, 2017

Copy link
Copy Markdown
Contributor Author

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.

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@markekraus
[feature] Match -> MatchExactly
5caf544
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Sep 26, 2017
View reviewed changes

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) left a comment

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@PowerShell PowerShell Team (PowerShell) deleted a comment from Microsoft Contribution License Agreements (msftclas) Sep 27, 2017
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit a8e8b1f into PowerShell:master Sep 27, 2017
@markekraus Mark Kraus (markekraus) mentioned this pull request Oct 5, 2017
beta.8 changelog draft #5006
Merged
@markekraus
Mark Kraus (markekraus) deleted the FixDefaultUserAgent branch January 19, 2018 18:57
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@markekraus @adityapatwardhan
Change UserAgent App WindowsPowerShell -> PowerShell (PowerShell#4914)
909a167 
* Change UserAgent App WindowsPowerShell -> PowerShell

* [Feature] Run Feature tests

* [feature] Address PR Feedback with more precise assertion

* [feature] Address PR Feedback and add PSUserAgent Test

* [feature] add tag to describe block

* [feature] Remove describe block Add pattern to User-Agent specific tests

* [feature] Address PF Feedback

Remove all assertions for User-Agent that are not specifically about the User-Agent header.

* [feature] Match -> MatchExactly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SteveL-MSFT Steve Lee (SteveL-MSFT) SteveL-MSFT approved these changes
@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
@iSazonov Ilya (iSazonov) iSazonov approved these changes
@dantraMSFT Dan Travison (dantraMSFT) Awaiting requested review from dantraMSFT
@PaulHigin Paul Higinbotham (PaulHigin) Awaiting requested review from PaulHigin
+1 more reviewer
@JamesWTruher James Truher (JamesWTruher) JamesWTruher approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Default User Agent Reports WindowsPowerShell

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@markekraus @JamesWTruher @SteveL-MSFT @adityapatwardhan @iSazonov @msftclas @msftgits
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.