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

httpapi: let httpapi plugin handle HTTPErrors other than 401 #43436

Merged
merged 8 commits into from
Aug 13, 2018

Conversation

Qalthos
Copy link
Contributor

@Qalthos Qalthos commented Jul 30, 2018

SUMMARY
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

httpapi

ANSIBLE VERSION
2.7

@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 feature This issue/PR relates to a feature 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. labels Jul 30, 2018
@ansibot
Copy link
Contributor

ansibot commented Jul 30, 2018

The test ansible-test sanity --test pylint [explain] failed with 4 errors:

lib/ansible/plugins/httpapi/eos.py:37:18: undefined-variable Undefined variable 'AnsibleConnectionFailure'
lib/ansible/plugins/httpapi/eos.py:38:55: undefined-variable Undefined variable 'exc'
lib/ansible/plugins/httpapi/nxos.py:34:18: undefined-variable Undefined variable 'AnsibleConnectionFailure'
lib/ansible/plugins/httpapi/nxos.py:35:47: undefined-variable Undefined variable 'exc'

click here for bot help

@ansibot ansibot added 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 30, 2018
))

try:
response_data = json.loads(response_data.getvalue())
Copy link
Contributor

Choose a reason for hiding this comment

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

This might fail on python 3 and json.loads expects string and getvalue() will return bytes .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On 3.5, yeah... I'll fix that

@gdpak
Copy link
Contributor

gdpak commented Jul 31, 2018

A bit off topic. Can you please also take care of below small change otherwise it is modifying headers passed to send() .

@@ -243,7 +243,9 @@ class Connection(NetworkConnectionBase):
         )
         url_kwargs.update(kwargs)
         if self._auth:
-            url_kwargs['headers'].update(self._auth)
+            headers = dict(url_kwargs['headers'])
+            headers.update(self._auth)
+            url_kwargs['headers'] = headers
         else:

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 31, 2018
@Qalthos Qalthos added networking Network category and removed needs_triage Needs a first human triage before being processed. labels Jul 31, 2018
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Jul 31, 2018
@Qalthos
Copy link
Contributor Author

Qalthos commented Jul 31, 2018

The last commit can stay or go... it does add a bit of complexity, but it also means that the whole business of dealing with HTTP status codes is customizable in a flexible way.

Edit: by 'The last commit', I am now referring to the commit following this comment, as it needed an edit.

but let httpapi plugin have a say on how it happens

response_text = response.read()
response_buffer = BytesIO()
response_buffer.write(response.read())
Copy link
Contributor

Choose a reason for hiding this comment

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

@Qalthos In case we were downloading big file then we were seeing update_auth() crash while trying to json.loads of response_data. so one idea was to not look at response_data in each sent request. so we update auth data inside http_plugin
CiscoDevNet/FTDAnsible@54ea3cb

with this, we don't need to read response in update_auth. So if you wish we can convert logic to send only response from send() like earlier and client can do read() or chunk read whatever they wish to ?

@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 Aug 11, 2018
@Qalthos Qalthos merged commit a3385a6 into ansible:devel Aug 13, 2018
@Qalthos Qalthos deleted the httpapi-update branch August 13, 2018 14:25
@dagwieers dagwieers added nxos Cisco NXOS community cisco Cisco technologies labels Feb 22, 2019
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 cisco Cisco technologies feature This issue/PR relates to a feature request. networking Network category nxos Cisco NXOS community stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants