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_uri: fixes #35487

Merged
merged 2 commits into from Feb 7, 2018
Merged

win_uri: fixes #35487

merged 2 commits into from Feb 7, 2018

Conversation

jborean93
Copy link
Contributor

@jborean93 jborean93 commented Jan 30, 2018

SUMMARY

Fix various aspects of the win_uri module.

Fixes #35413 - no support for TLS 1.1 and 1.2
Fixes #34698 - no json return object
Fixes #32037 - set both string and dict for body (json)
Fixes #31981 - cannot set headers
Fixes #31166 - not validating non 2xx error codes

Also fixes (no issues currently raised for it)

  • Disabling redirection works without an error occuring
  • Idempotency when creating a file based on the content
  • Integration tests (more work is required to move away from an external dependency)
  • Ability to set a password for client certificate (required when not using CredSSP)
  • Actually return header key/value pairs instead of a list of header keys like before

There are some breaking changes here but I thought it would be good to rip them out considering it is a preview module. The stuff I have removed are;

  • Changed from Invoke-WebRequest to the .net HttpWebRequest in order to support redirection properly
  • Because of the above, some return objects are not returned anymore
  • Changed the maximum redirection to 50 to reflect the default of HttpWebRequest
  • use_basic_parsing does nothing anymore so a short warning is set saying it is no longer used and will be removed
  • Removed some return values which are just a copy of the input values

Unfortunately the scope of changes means I won't be able to backport it.

ISSUE TYPE
  • Feature Pull Request
  • Bugfix Pull Request
COMPONENT NAME

win_uri

ANSIBLE VERSION
2.5

@ansibot
Copy link
Contributor

ansibot commented Jan 30, 2018

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. feature_pull_request module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community labels Jan 30, 2018
@jborean93 jborean93 removed the needs_triage Needs a first human triage before being processed. label Jan 30, 2018
Content-Length { $client.ContentLength = $header.Value }
Content-Type { $client.ContentType = $header.Value }
Expect { $client.Expect = $header.Value }
Date { $client.Date = $hader.Value }
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo $hader.Value

@@ -0,0 +1,3 @@
---
test_uri_path: C:\ansible\win_uri
httpbin_host: httpbin.org
Copy link
Contributor

Choose a reason for hiding this comment

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

So I was told we didn't want to do test against Internet infrastructure, that's why we have this long-standing issue open regarding having access from the Windows VM's to the httptester infrastructure that Docker images are using.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't, this is a wait and see thing. If we come across any recurring issues we will disable the tests in CI but until we have added the code to forward the httptester container ports to the Windows host this is better than nothing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fine by me. Although enabling httptester wasn't supposed to be so hard either ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are getting close, there is a PR out there to install the SSH server on the Windows hosts and @mattclay is looking into creating a remote port forwarder to the Windows host with this SSH server for certain tests. We can do all this manually but the trick is getting it to work in CI and be stable.

Once that is in, it will be as simple as changing this variable to be localhost:port.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch that sounds messy :-\ And wouldn't that be tricky to run from your laptop ?

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 30, 2018
use_basic_parsing:
description:
- As of Ansible 2.5, this option is no longer valid and cannot be changed from C(yes).
- This will be removed in Ansible 2.7
- This module relies upon 'Invoke-WebRequest', which by default uses the Internet Explorer Engine to parse a webpage.
Copy link
Contributor

Choose a reason for hiding this comment

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

L67-L69 can be removed from the documentation.

@jhawkesworth
Copy link
Contributor

Looks good to me now.

@jborean93
Copy link
Contributor Author

I've made the changes requested by the review and will merge in as the community freeze will be upon us.

@jborean93 jborean93 merged commit a0178b7 into ansible:devel Feb 7, 2018
@jborean93 jborean93 deleted the win_uri_fixes branch February 7, 2018 10:25
@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 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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. net_tools Net-tools category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. test This PR relates to tests. windows Windows community
Projects
None yet
4 participants