X Tutup
Skip to content

Add HttpVersion parameter to Invoke-WebRequest cmdlet#15853

Merged
daxian-dbw merged 20 commits intoPowerShell:masterfrom
hayhay27:add-http-version-parameter
Nov 2, 2021
Merged

Add HttpVersion parameter to Invoke-WebRequest cmdlet#15853
daxian-dbw merged 20 commits intoPowerShell:masterfrom
hayhay27:add-http-version-parameter

Conversation

@hayhay27
Copy link
Copy Markdown
Contributor

@hayhay27 hayhay27 commented Aug 1, 2021 •
edited
Loading

PR Summary

Fix #12641

  • Add HttpVersionCompletionsAttribute with completions 1.0, 1.1, 2.0, 3.0
  • Handle input conversions from string, int and double to Version (generalize ArgumentToVersionTransformationAttribute from Set-StrictMode)
  • Enrich Verbose mesasge for request with http version and uri
  • Add pester tests for HttpVersion completions
  • Add xUnit tests for ArgumentToVersionTransformationAttribute

PR Context

This PR addressed to issue #12641
At the moment impossible make requests to network resources served only with HTTP/2 cause of default http version in HttpRequestMessage.Version=1.1

Changes in this PR allow to set http version during invocation Invoke-RestMethod or Invoke-WebRequest

This is my first PR to PowerShell. I have a couple of questions about tests. How and where to implement (at least point me right place please).

PR Checklist

@ghost ghost assigned daxian-dbw Aug 1, 2021
@ghost
Copy link
Copy Markdown

ghost commented Aug 1, 2021 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

iSazonov
iSazonov reviewed Aug 1, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for test, I think we have no need to do real HTTP requests with specified version since we utilize .Net Runtime features. I think it would enough if you did manual checks and shared results.
Nevertheless, we must verify that (1) the parameter accepts well-know values without throw, (2) transformation attribute works (it could be xUnit tests), (3) tab-completion works.

@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Aug 1, 2021
@hayhay27
Copy link
Copy Markdown
Contributor Author

hayhay27 commented Aug 1, 2021

As for test, I think we have no need to do real HTTP requests with specified version since we utilize .Net Runtime features. I think it would enough if you did manual checks and shared results.
Nevertheless, we must verify that (1) the parameter accepts well-know values without throw, (2) transformation attribute works (it could be xUnit tests), (3) tab-completion works.

  1. do you mean a pester test?
  2. done with xunit
  3. done with xunit

iSazonov
iSazonov reviewed Aug 2, 2021
View reviewed changes
@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Aug 5, 2021
@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch from f2c9643 to 5cd00b7 Compare August 9, 2021 08:13
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 19, 2021
@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch from 6fd9a05 to cb4cb12 Compare August 21, 2021 11:25
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Aug 21, 2021
@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch from 4645764 to 7897ad8 Compare August 21, 2021 12:06
@hayhay27 hayhay27 changed the title WIP: Add HttpVersion parameter to Invoke-WebRequest cmdlet Add HttpVersion parameter to Invoke-WebRequest cmdlet Aug 21, 2021
iSazonov
iSazonov reviewed Aug 23, 2021
View reviewed changes
@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch from 7897ad8 to 753ab24 Compare August 25, 2021 06:58
@hayhay27
Copy link
Copy Markdown
Contributor Author

hayhay27 commented Aug 25, 2021

I have no idea about failed tests )
What is that? )

@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch 2 times, most recently from d3a3a78 to 84e6695 Compare August 25, 2021 17:47
@hayhay27 hayhay27 marked this pull request as ready for review August 25, 2021 18:19
@hayhay27 hayhay27 requested a review from iSazonov August 26, 2021 16:03
@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented Aug 27, 2021

@hayhay27 Please update the PR description to address latest code changes.

hayhay27
hayhay27 commented Aug 29, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor Author

@hayhay27 hayhay27 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR description to address latest code changes.

done

@hayhay27
Copy link
Copy Markdown
Contributor Author

hayhay27 commented Sep 2, 2021

while we keep silence

I thought with your blessing I can update docs for this PR

hayhay27 and others added 6 commits October 8, 2021 19:10
@hayhay27 hayhay27 force-pushed the add-http-version-parameter branch from 04bcb4f to 83ce173 Compare October 8, 2021 16:10
@hayhay27
Add HttpVersion parameter to Invoke-WebRequest
27b46b3 
fixes by review
- remove HttpVersionUtils
- refactor pester test
@hayhay27
Copy link
Copy Markdown
Contributor Author

hayhay27 commented Oct 8, 2021

@rjmholt Thanks a lot!
I applied all your comments

@hayhay27 hayhay27 requested a review from rjmholt October 8, 2021 17:00
@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Oct 15, 2021

@rjmholt Can you please take another look?

daxian-dbw
daxian-dbw requested changes Nov 2, 2021
View reviewed changes
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 2, 2021
@hayhay27
Add HttpVersion parameter to Invoke-WebRequest
2fb1df8 
fixes by review
- extend version transformation via method override instead of ctor injection
- clean up code
@pull-request-quantifier-deprecated
Copy link
Copy Markdown

pull-request-quantifier-deprecated bot commented Nov 2, 2021

This PR has 254 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!

Quantification details 

Label      : Large
Size       : +186 -68
Percentile : 65.4%

Total files changed: 10

Change summary by file extension:
.cs : +145 -66
.resx : +2 -2
.ps1 : +39 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detetcted.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.

Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 2, 2021
@hayhay27 hayhay27 requested review from daxian-dbw and removed request for rjmholt November 2, 2021 21:35
daxian-dbw
daxian-dbw approved these changes Nov 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@daxian-dbw daxian-dbw merged commit 674ef88 into PowerShell:master Nov 2, 2021
@daxian-dbw
Copy link
Copy Markdown
Member

daxian-dbw commented Nov 2, 2021

@hayhay27 Thank you for your contribution!

@SteveL-MSFT SteveL-MSFT mentioned this pull request Nov 18, 2021
Merged
22 tasks
@ghost
Copy link
Copy Markdown

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Dec 16, 2021
Closed
@kevinoid kevinoid mentioned this pull request Dec 28, 2021
Closed
TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@hayhay27 @TrapGodBrim
Add -HttpVersion parameter to web cmdlets (PowerShell#15853)
cc27f06
daxian-dbw added a commit to daxian-dbw/PowerShell that referenced this pull request May 16, 2022
@daxian-dbw
Redo the change that was reverted by PowerShell#15853
cd1f770
@daxian-dbw daxian-dbw mentioned this pull request May 16, 2022
Merged
7 tasks
TravisEz13 pushed a commit that referenced this pull request May 17, 2022
@daxian-dbw
Redo the change that was reverted by #15853 (#17357)
391e080
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@daxian-dbw daxian-dbw daxian-dbw approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@JamesWTruher JamesWTruher Awaiting requested review from JamesWTruher

@anmenaga anmenaga Awaiting requested review from anmenaga

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13

+1 more reviewer

@rjmholt rjmholt rjmholt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@daxian-dbw daxian-dbw

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Large WG-Cmdlets general cmdlet issues WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

invoke-restmethod does not support http/2

4 participants

@hayhay27 @iSazonov @daxian-dbw @rjmholt
X Tutup