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

Fix #57 - add headers parameter to methods create, update and delete #58

Closed
wants to merge 4 commits into from

Conversation

positiveEV
Copy link

@positiveEV positiveEV commented Aug 3, 2022

  • Add headers parameter to methods create, update and delete
  • Pass headers to internal method _query

@cmeissner
Copy link
Member

cmeissner commented Aug 3, 2022

@positiveEV thank you for submitting your first PR. To get it merged you need to fix some small things.

  1. Format the title sth like "Fix <issue-id> - short description of issue"
  2. Add a description what you have changed
  3. Add a change log entry use changelog command (came from requirements-dev.txt)
  4. Add a test case for the use case which is covered by your PR
    As you can't provide any steps to reproduce the issue the test case can only check the success but not the failure state

After that I think we can merge the PR as it seems not to interfere the existing test cases.

@cmeissner cmeissner self-requested a review August 3, 2022 16:31
@cmeissner cmeissner added enhancement New feature or request good first issue Good for newcomers labels Aug 3, 2022
@cmeissner cmeissner changed the title add 'hearders' arg to methods create, update and delete Fix #57 - add header parameter to methods create, update and delete Aug 13, 2022
@cmeissner cmeissner changed the title Fix #57 - add header parameter to methods create, update and delete Fix #57 - add headers parameter to methods create, update and delete Aug 13, 2022
Signed-off-by: Christian Meißner <cme+github@meissner.sh>
Copy link
Member

@cmeissner cmeissner left a comment

Choose a reason for hiding this comment

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

LGTM but test case is still missing

@positiveEV
Copy link
Author

Sorry, for not coming back at you earlier.
Regarding the test case if I write a ensure_address.py, and pass a headers arg to create_entity, will it be ok?

@cmeissner
Copy link
Member

cmeissner commented Aug 13, 2022

No matter. I had some time today to fix some points of the list. Two last things:

  1. Can you please add headers parameter also to get_entity method?
  2. Regarding the test case. As we can't reproduce the issue the test should test the documented feature to change the output from json to xml with Content-Type: application/xml. Here would be a ensure_address (CRUD test) a good idea. But instead of working with json responses work with xml.

Copy link
Member

@cmeissner cmeissner left a comment

Choose a reason for hiding this comment

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

This does not work out. If you run pi.get_entity(controller='subnets', controller_path='1', headers={'Content-Type': 'application/xml'}) (after applying the requested change in get_entity method) you run into an exception.

pi.get_entity(controller='subnets', controller_path='1', headers={'Content-Type': 'application/xml'})
Traceback (most recent call last):
  File "/home/cme/.pyenv/versions/phpypam_3.9.11/lib/python3.9/site-packages/requests/models.py", line 971, in json
    return complexjson.loads(self.text, **kwargs)
  File "/home/cme/.pyenv/versions/3.9.11/lib/python3.9/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/home/cme/.pyenv/versions/3.9.11/lib/python3.9/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/home/cme/.pyenv/versions/3.9.11/lib/python3.9/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/cme/Development/github/username/phpypam/phpypam/core/api.py", line 170, in get_entity
    return self._query(token=self._api_token, method=GET, path=_path, params=_params, headers=_headers)
  File "/home/cme/Development/github/username/phpypam/phpypam/core/api.py", line 121, in _query
    result = resp.json()
  File "/home/cme/.pyenv/versions/phpypam_3.9.11/lib/python3.9/site-packages/requests/models.py", line 975, in json
    raise RequestsJSONDecodeError(e.msg, e.doc, e.pos)
requests.exceptions.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

This is related to result = resp.json() in _query method. You need to refactor this method, otherwise all other methods will break if you use Content-Type: application/xml.


if _controller_path:
_path = '{}/{}'.format(_path, _controller_path)

return self._query(token=self._api_token, method=GET, path=_path, params=_params)
return self._query(token=self._api_token, method=GET, path=_path, params=_params, headers=None)
Copy link
Member

Choose a reason for hiding this comment

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

This is an error, you need to pass _headers here.

Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment

@github-actions github-actions bot added the Stale label Nov 20, 2023
@cmeissner
Copy link
Member

This PR was closed because it has been stalled for 15 days with no activity.

@cmeissner cmeissner closed this Dec 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants