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

add a headers arg to create_entity() #57

Open
positiveEV opened this issue Aug 1, 2022 · 2 comments
Open

add a headers arg to create_entity() #57

positiveEV opened this issue Aug 1, 2022 · 2 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@positiveEV
Copy link

Hi,
Thank you for this nice wrapper.

Describe the bug
Sometimes I run into this error phpipam/phpipam#3177
and as stated in this issue, I have to pass {"content-type: "application/x-www-form-urlencoded"}
in the headers to solve it.
Currently create_entity() does not have a 'headers' argument, so I have to call _query() directly to pass the header to the request.

Expected behavior
It is a bite dirty to have to call an internal function directly, so I would like to
be able to pass an 'headers' arg to create_entity(), and also to update_entity() and delete_entity for consistency I guess.

Versions:

  • python 3.10
  • phpypam 1.0.2
@positiveEV positiveEV added the bug Something isn't working label Aug 1, 2022
@cmeissner
Copy link
Member

cmeissner commented Aug 2, 2022

Hi @positiveEV,

Thank you for bringing this issue to our attention. We developed the library strictly aligned to the existing api documantation. The content-type headers is mentioned in output format section only. It seems that the mentioned issue phpipam/phpipam#3177 was never answered by phpipam developers and it is only a workaround.
We also developed this library with the requirement of having json output available. Therefor we currently did not provide any option to set headers in any method.

From my point of view this is a RFE not a bug. I'm not very sure when I can start working on this feature. I also miss some steps to reproduce the issue and to write a needed test for such a new feature.

As you already use the _query method to achieve what you need may I ask you to provide a PR with the needed changes?

Kind regards,
Christian

@cmeissner cmeissner added enhancement New feature or request help wanted Extra attention is needed and removed bug Something isn't working labels Aug 2, 2022
@positiveEV
Copy link
Author

As you ask I created a PR #58 to add the headers arg in methods modifying entities in phpIPAM.

Unfortunately I can tell you how to reproduce the issue, as I not able to reproduce it myself.
The api was returning me a http error code 400 with the message "Subnet Id must be numeric".
I found the workaround in phpipam/phpipam#3177, I tried it to create one ip and it work.
Then I tried again to create an ip without the content-type header param and it also worked.
Since then I did not have to use this workaround again.
But knowing that the header might be needed, I will integrate it to my script before pushing it into prod.

So I do not know if you should merge the PR or not, because as you wrote this is just a workaround for what seems to be a bug in the phpIPAM api. So may be I should reopen the issue that I mentioned in phpIPAM.
Nevertheless if you merge the PR, I do not think that it will an impact on code quality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants