Skip to content
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

win_get_url: Add use_proxy, headers and timeout #26612

Merged
merged 2 commits into from
Aug 29, 2017

Conversation

dagwieers
Copy link
Contributor

@dagwieers dagwieers commented Jul 10, 2017

SUMMARY

This PR includes:

  • Add use_proxy parameter
  • Add headers parameter
  • Renamed username to url_username
  • Renamed password to url_password
  • Simplify logic
  • Create separate CheckModified-File function
  • Now use -LiteralPath instead of -Path
  • Clean up documentation

This relates to #20160
This fixes #19573

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_get_url

ANSIBLE VERSION

v2.4

@ansibot ansibot added affects_2.4 This issue/PR affects Ansible v2.4 feature_pull_request needs_triage Needs a first human triage before being processed. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 10, 2017
nitzmahone added a commit that referenced this pull request Jul 10, 2017
* invalid drive letter breaks new Powershell path validation (this is a separate issue that needs to be solved)
* test also exposes a minor bug in win_get_url where a nonexistent directory in the root of a drive that exists passes the "does the parent dir exist" check and causes DownloadFile to bomb (this should be fixed by dag's rewrite of win_get_url in #26612)
@alikins alikins removed the needs_triage Needs a first human triage before being processed. label Jul 10, 2017
@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 10, 2017
@dagwieers dagwieers changed the title win_get_url: Add use_proxy, headers and timeout WIP: win_get_url: Add use_proxy, headers and timeout Jul 10, 2017
@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 10, 2017
@dagwieers dagwieers force-pushed the win_get_url-clean branch 13 times, most recently from ae99c29 to c95650a Compare July 11, 2017 00:16
@dagwieers dagwieers changed the title WIP: win_get_url: Add use_proxy, headers and timeout win_get_url: Add use_proxy, headers and timeout Jul 11, 2017
@ansibot ansibot removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 11, 2017
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 24, 2017
@ansibot ansibot removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 4, 2017
@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 16, 2017
Copy link
Contributor

@jborean93 jborean93 left a comment

Choose a reason for hiding this comment

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

Proxy stuff works but there are a few others issues I came across. If you can fix those I'll do a speedy merge before the freeze.

Also looks like you need to rebase this.

}

if ($timeout) {
$webRequest.Timeout = $timeout * 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Timeout is not a valid property of WebClient https://msdn.microsoft.com/en-us/library/system.net.webclient(v=vs.110).aspx. Might be best to remove this new feature so we can get the rest in for 2.4 but I'll let you have a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is not a WebClient object, it is a HttpWebRequest object, and it does have a Timeout property. I also see a ConnectTimeout property, which we also could add. https://msdn.microsoft.com/en-us/library/system.net.httpwebrequest(v=vs.110).aspx

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry ignore this line, line 88 is the one that is the invalid property.

}

if ($timeout) {
$webClient.Timeout = $timeout * 1000
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So here we do have a WebClient object. It's strange that it worked fine for me, I'll remove the timeout here.

$params = Parse-Args $args -supports_check_mode $true
$check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false

$url = Get-AnsibleParam -obj $params -name "url" -type "str" -failifempty $true
$dest = Get-AnsibleParam -obj $params -name "dest" -type "path" -failifempty $true
$timeout = Get-AnsibleParam -obj $params -name "timeout" -type "int" -default 10
$headers = Get-AnsibleParam -obj $params -name "headers" -type "dict"
Copy link
Contributor

Choose a reason for hiding this comment

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

You will need to set the default to @{} so when call GetEnumerator() in the methods above it won't fail if a user doesn't pass anything in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed that the default would adapt to the type in Ansible, but we don't do that anywhere.
I learned something new today :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

dict isn't a real type right now, even if it was I'm not sure if it should. There may be a reason where we need to know if it is null vs when it is an empty dict.

$fileLastMod = ([System.IO.FileInfo]$dest).LastWriteTimeUtc
$webLastMod = $null
Download-File -result $result -url $url -dest $dest -credentials $credentials `
-headers $headers -timeout $timeout -use_proxy $use_proxy -proxy_server $proxy_server `
Copy link
Contributor

Choose a reason for hiding this comment

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

s/proxy_server/proxy/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch !

@@ -19,120 +20,194 @@
# WANT_JSON
# POWERSHELL_COMMON

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add $ErrorActionPreference = 'Stop' to this, would have helped you to pick up some of the errors below and keeps it more inline with other PS modules we have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 28, 2017
@dagwieers dagwieers force-pushed the win_get_url-clean branch 2 times, most recently from d104539 to b33d3ad Compare August 28, 2017 22:25
This PR includes:
- Add use_proxy parameter
- Add timeout parameter
- Add headers parameter
- Simplify logic
- Create separate CheckModified-File
- Now use -LiteralPath instead of -Path
- Clean up documentation
@ansibot ansibot removed the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Aug 28, 2017
This PR includes:
- Add use_proxy parameter
- Add timeout parameter
- Add headers parameter
- Simplify logic
- Create separate CheckModified-File
- Now use -LiteralPath instead of -Path
- Clean up documentation
@dagwieers
Copy link
Contributor Author

Typo fixed.

@jborean93
Copy link
Contributor

Thanks the work, merging in for 2.4

@jborean93 jborean93 merged commit 61d2201 into ansible:devel Aug 29, 2017
prasadkatti pushed a commit to prasadkatti/ansible that referenced this pull request Oct 1, 2017
* win_get_url: Add use_proxy, headers and timeout

This PR includes:
- Add use_proxy parameter
- Add timeout parameter
- Add headers parameter
- Simplify logic
- Create separate CheckModified-File
- Now use -LiteralPath instead of -Path
- Clean up documentation

* win_get_url: Add use_proxy, headers and timeout

This PR includes:
- Add use_proxy parameter
- Add timeout parameter
- Add headers parameter
- Simplify logic
- Create separate CheckModified-File
- Now use -LiteralPath instead of -Path
- Clean up documentation
@ctal80
Copy link

ctal80 commented Jan 30, 2018

Hi,
I'm not able to use the headers parameter (e.g. headers: 'X-JFrog-Art-Api: AKCp5Z2Y3oz3MRbmpthVW4omH3xymFRzGkrFfacGSbAU7abGDF5nGe6WXkJsM6UFeZqYLmeCx') and getting the following error:

FAILED! => {"changed": false, "module_stderr": "An error occurred while creating the pipeline.\r\n + CategoryInfo : NotSpecified: (:) [], ParentContainsErrorRecordE \r\n xception\r\n + FullyQualifiedErrorId : RuntimeException\r\n \r\nException calling "Add" with "2" argument(s): "The parameter 'name' cannot be \r\nan empty string.\r\nParameter name: name"\r\nAt line:94 char:9\r\n+ $extWebClient.Headers.Add($header.Name, $header.Value)\r\n+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~\r\n + CategoryInfo : NotSpecified: (:) [], ParentContainsErrorRecordE \r\n xception\r\n + FullyQualifiedErrorId : ArgumentException\r\n \r\n\r\n", "module_stdout": "", "msg": "MODULE FAILURE", "rc": 1}
to retry, use: --limit @/home/ad_talcohen/ansibleEval/playbooks/win_package_demo.retry

Can you please advice?

Thanks
Tal

@dagwieers
Copy link
Contributor Author

@ctal80 Please open a new issue, rather than hijack a closed PR. BTW according to the docs, the input expects a dictionary, but you provide a string.

PS If you open an issue, please provide all details because your current comment is not sufficient to understand what you're doing and how the error relates to what you're doing.

@ctal80
Copy link

ctal80 commented Jan 30, 2018

Hi,

I'm sorry for hijacked the closed PR.

I changed the parameter from string to dictionary e.g {X-JFrog-Art-Api: AKCp5Z2Y3oz3MRbmpthVW4omH3xymFRzGkrFfacGSbAU7abGDF5nGe6WXkJsM6UFeZqYLmeCx}

and now it's working properly and i'm able to download the file.

Thanks

Tal

@dagwieers
Copy link
Contributor Author

@ctal80 There's still use in opening an issue so we can fix the error-message and better test for the input.

@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 5, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.4 This issue/PR affects Ansible v2.4 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. support:core This issue/PR relates to code supported by the Ansible Engineering Team. windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to pass in the "headers:" parameter into win_get_url (unlike get_url)
6 participants