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

Windows - Add common util for web requests #54759

Merged
merged 10 commits into from
Jul 21, 2019

Conversation

jborean93
Copy link
Contributor

@jborean93 jborean93 commented Apr 3, 2019

SUMMARY

Adds a common URL handler for PowerShell modules. It tries to combine common elements like shared module arg spec, reduce code duplication for the somewhat convoluted WebRequest class in .NET and add the ability to easily add common module options for URL based modules in the future.

Things I know that still need to be fixed/solved before this can move away from a WIP PR;

  • Fix validate-modules to load other PS and C# module utils so the module arg spec can be from a util. Currently it fails because we don't load them Edit: Done with Load Ansible module_utils for ps_argspec validator #58571
  • Figure out how we want to document the shared values and when they were added. Right now it just omits the version_added component in the shared doc fragment because some options were present/added in some modules but not others - Doc fragments are more versatile than I expected, have been able to preserve the version_added for specific modules while still sharing the docs as much as possible.
  • Whether Invoke-WithWebRequest is too complicated with it's use of a ScriptBlock
  • Whether Register-AnsibleWebRequestParams is something we want to use. Unfortunately we cannot take advantage of splatting from $module.Params and this was the next best thing I could find that allowed the module to ignore options that were implemented inside. - Alternative option was chosen where the module (if passed in) would provide a backup/default value for parameters not explicitly passed in
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_get_url
win_uri

@ansibot
Copy link
Contributor

ansibot commented Apr 3, 2019

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.8 This issue/PR affects Ansible v2.8 feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. 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. windows Windows community labels Apr 3, 2019
@ansibot

This comment has been minimized.

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Apr 3, 2019
@bcoca bcoca removed the needs_triage Needs a first human triage before being processed. label Apr 9, 2019
@ansibot ansibot added 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 Apr 17, 2019
@ansibot
Copy link
Contributor

ansibot commented Jun 4, 2019

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 20, 2019
@jborean93 jborean93 changed the title WIP - Windows - Add common util for web requests Windows - Add common util for web requests Jul 1, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. ci_verified Changes made in this PR are causing tests to fail. 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 Jul 1, 2019
@ansibot

This comment has been minimized.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jul 1, 2019
@ansibot ansibot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 5, 2019
@jhawkesworth
Copy link
Contributor

Another impressive piece of work.
I haven't spotted anything that looks like it needs changing, other than pslint/ignore.txt conflict.

The only thing I can think of is maybe to benchmark against existing modules, although I can't think of a reason why performance might change materially (and network conditions are much more likely to affect actual execution time in use) but more testing before merging a big refactoring allways improves confidence that the change will stick.

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jul 9, 2019
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jul 15, 2019
@ansibot
Copy link
Contributor

ansibot commented Jul 15, 2019

The test ansible-test sanity --test future-import-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/url_windows.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test metaclass-boilerplate [explain] failed with 1 error:

lib/ansible/plugins/doc_fragments/url_windows.py:0:0: missing: __metaclass__ = type

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/windows/win_get_url.ps1:0:0: E325 Argument 'client_cert' in argument_spec defines type as 'str' but documentation defines type as 'path'
lib/ansible/modules/windows/win_uri.ps1:0:0: E325 Argument 'client_cert' in argument_spec defines type as 'str' but documentation defines type as 'path'

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 15, 2019
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jul 15, 2019
Copy link
Member

@nitzmahone nitzmahone left a comment

Choose a reason for hiding this comment

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

Looks great, one little thing to think about with the default handling on explicit HttpAgent arg vs headers. Much nicer setup in general. I don't love the pre-existing continuation pattern of Invoke-WithWebRequest, but it seems to work OK for most things right now- something that could probably be revisited later if there's a good reason.

@ansibot
Copy link
Contributor

ansibot commented Jul 21, 2019

@ansibot ansibot added the docs This issue/PR relates to or includes documentation. label Jul 21, 2019
@jborean93
Copy link
Contributor Author

Merging to get this out into people's installs so we can hopefully detect any bugs before the 2.9 release. Thanks everyone for the reviews.

@jborean93 jborean93 merged commit 015119d into ansible:devel Jul 21, 2019
@jborean93 jborean93 deleted the win-webrequest branch July 21, 2019 22:42
@ansible ansible locked and limited conversation to collaborators Aug 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. new_plugin This PR includes a new plugin. 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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants