netbox-community / pynetbox

Python API client library for Netbox.
Apache License 2.0
580 stars 172 forks source link

Include Auth Token when Getting API Version #613

Closed terraflubb closed 5 months ago

terraflubb commented 6 months ago

Fixes: #612

When we get the API version, we aren't including the auth token if we have one, and as a result the request fails (as of Netbox 4.0.0).

I simply copied the header-building from get_status below to make sure I was following the convention, since it does exactly what we'd like.

I tested this locally and it resolved my issue, but I haven't tested it fully throughout our API integrations (as I imagine there will be more as we move to Netbox 4.0.0) so I'm not sure if there are any other side effects I haven't considered.

markkuleinio commented 6 months ago

@terraflubb Please also add the missing token to the request in the version property in api.py (see status() method there)

markkuleinio commented 6 months ago

@terraflubb And, the OpenAPI functionality suffers the same problem in the same files, so if you could tackle it at the same time?

terraflubb commented 6 months ago

@markkuleinio Good call! I believe I've made the changes you've requested.

terraflubb commented 6 months ago

The checks are failing on all kinds of files I didn't touch. I guess this is the first PR since that check was added?

I can force-push it away if it's beyond the scope of this PR, but I added the 12 newlines so that black would be satisfied and it will pass the checks.

markkuleinio commented 6 months ago

Maybe @arthanson could take a look at the the pipelines and publish this as well? I don't see any checks being applied to this or the previous PR. There were lots of failed jobs also last time the maintainer committed changes.

terraflubb commented 6 months ago

For reference, here's the link to the failed job, it looks like it was approved by @abhi1693 : https://github.com/netbox-community/pynetbox/actions/runs/9007698674

kotso commented 6 months ago

Someone please merge it.

markkuleinio commented 6 months ago

As I see the NetBox project is considering notifying the issue submitters in one week if no action (https://github.com/netbox-community/netbox/issues/16064), I'm tagging @arthanson and @abhi1693 here again, if they could get someone to move this PR forward. Thanks in advance!

karolyczovek commented 6 months ago

API change also affects lookups, with your changes I see the following:

modules working fine, for both 3.7.8 and 4.0+

lookups are failing against both 3.7.8 and 4.0+

An unhandled exception occurred while running the lookup plugin 'netbox.netbox.nb_lookup'. Error was a <class 'TypeError'>, original message: Api.init() got an unexpected keyword argument 'private_key'. Api.init() got an unexpected keyword argument 'private_key'"}

markkuleinio commented 6 months ago

No idea what is "lookup" but searching for nb_lookup brings this:

https://docs.ansible.com/ansible/latest/collections/netbox/netbox/nb_lookup_lookup.html

private_key string | (DEPRECATED) - NetBox 2.11 and earlier only The private key as a string. Mutually exclusive with key_file.

karolyczovek commented 6 months ago

No idea what is "lookup" but searching for nb_lookup brings this:

https://docs.ansible.com/ansible/latest/collections/netbox/netbox/nb_lookup_lookup.html

Exactly - so this will be a pynetbox<>ansible compatibility. I don't use any private_key attribute on the lookups

markkuleinio commented 6 months ago

Exactly - so this will be a pynetbox<>ansible compatibility.

You are correct: Ansible is apparently creating a pynetbox.Api instance with incorrect arguments.

(Unrelated to this PR)

terraflubb commented 6 months ago

I'm having trouble reproducing whatever the holdup is, so I don't know what to do in order to help move this along.

I fixed the linting errors (on code I hadn't touched, so it was broken before the check was enabled) and when I run the tests locally as best as I can... (aside am I missing some "how to contribute" doc? I had to reverse-engineer this from the Github workflow). I didn't bother testing it on every Python / Netbox version in the matrix.

For somebody's future searchability (or so somebody can correct my way of testing things)

# in pynetbox source dir, with whatever tool you use to manage
# Python versions already happening
python -m venv pynetstuff
source pynetstuff/bin/activate
pip install -r requirements-dev.txt . 
pytest --netbox-versions=3.7.8

Anyway, when I do that (or just with 3.7) I get:

============================================= test session starts =============================================
platform linux -- Python 3.11.2, pytest-8.2.1, pluggy-1.5.0
rootdir: /home/where_my_code_is/pynetbox
plugins: docker-3.1.1
collected 204 items                                                                                           

tests/integration/test_dcim.py .........................................................                [ 27%]
tests/integration/test_ipam.py ..........................................                               [ 48%]
tests/test_api.py ......                                                                                [ 51%]
tests/test_app.py ...                                                                                   [ 52%]
tests/test_circuits.py ..............                                                                   [ 59%]
tests/test_tenancy.py ......                                                                            [ 62%]
tests/test_users.py .............                                                                       [ 69%]
tests/test_virtualization.py ...............                                                            [ 76%]
tests/test_wireless.py ....                                                                             [ 78%]
tests/unit/test_detailendpoint.py ..                                                                    [ 79%]
tests/unit/test_endpoint.py ............                                                                [ 85%]
tests/unit/test_extras.py ...                                                                           [ 86%]
tests/unit/test_query.py ...                                                                            [ 88%]
tests/unit/test_request.py ..                                                                           [ 89%]
tests/unit/test_response.py ......................                                                      [100%]

======================================= 204 passed in 112.61s (0:01:52) =======================================

Tests are green!

So... where is this new failure and what does it have to do with Ansible? Something in the docker image that is used for testing?

If I could reproduce it, I'd try to fix it, but at the moment I guess this (and thus, us migrating to Netbox 4.0, short of running our own fork) is just stuck?

We really depend on Netbox / pynetbox and would like to contribute, but this process is a little bit frustrating. I had a fix for this 2 weeks ago and we can't even get the workflow approved. :face_with_diagonal_mouth: I'm happy to do whatever it takes to make this as easy as possible to merge, but I'm lacking direction here.

davidmehren commented 6 months ago

It's the nb_lookup plugin that breaks, at this place: https://github.com/netbox-community/ansible_modules/blob/devel/plugins/lookup/nb_lookup.py#L431

In my case, it was because I directly installed (using pip) the branch of this PR into my Ansible venv. This causes the pynetbox package metadata to contain some kind of low version number generated from the Git commit, presumably because it is not a tagged version. The check if Version(version("pynetbox")) < Version("7.0.0"): in nb_lookup therefore fails to detect that the version of pynetbox is actually more recent than 7.0.0 and uses the wrong constructor for pynetbox.api.

If you instead use pip to install a stable version of pynetbox and then manually apply the changes of this PR, everything works correctly.

So from my perspective, this is just a (highly annoying) packaging quirk and has nothing to do with your code.

karolyczovek commented 6 months ago

You're right, just cross-checked. If I manually patch the installed lib it just works fine.

terraflubb commented 6 months ago

I hope this isn't considered spamming / harassing the maintainers, but it has been 10 days since they were last pinged. I get drowned in Github notifications regularly, so I understand how it might have fallen off their radar.

Could either @arthanson and @abhi1693 please move this along or let me know what else is needed?

aswen commented 5 months ago

What is stopping the repo owners from merging this PR?

abhi1693 commented 5 months ago

I'm not a maintainer anymore. You can ping others for assistance. I looked into it 2 weeks ago and the changes looked fine to me but I'll leave this up to other maintainers that would like to help with this going forward.

terraflubb commented 5 months ago

I'm not a maintainer anymore. You can ping others for assistance. I looked into it 2 weeks ago and the changes looked fine to me but I'll leave this up to other maintainers that would like to help with this going forward.

Thanks for letting us know, @abhi1693, do you happen to know who is responsible for this repo now? Is it only @arthanson now?

digi-talo commented 5 months ago

Can this be merged now? What is taking so long? @arthanson

seb-plg commented 5 months ago

Merge please

goebelmeier commented 5 months ago

Superseded by #616

terraflubb commented 5 months ago

Good news everybody! @jeffgdotorg merged @btriller's superior solution #616 ! We can finally upgrade Netbox!