-
Notifications
You must be signed in to change notification settings - Fork 173
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
Include Auth Token when Getting API Version #613
Include Auth Token when Getting API Version #613
Conversation
@terraflubb Please also add the missing token to the request in the |
@terraflubb And, the OpenAPI functionality suffers the same problem in the same files, so if you could tackle it at the same time? |
@markkuleinio Good call! I believe I've made the changes you've requested. |
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 |
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. |
For reference, here's the link to the failed job, it looks like it was approved by @abhi1693 : |
Someone please merge it. |
As I see the NetBox project is considering notifying the issue submitters in one week if no action (netbox-community/netbox#16064), I'm tagging @arthanson and @abhi1693 here again, if they could get someone to move this PR forward. Thanks in advance! |
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'"} |
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. |
You are correct: Ansible is apparently creating a (Unrelated to this PR) |
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
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. 🫤 I'm happy to do whatever it takes to make this as easy as possible to merge, but I'm lacking direction here. |
It's the In my case, it was because I directly installed (using pip) the branch of this PR into my Ansible venv. 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. |
You're right, just cross-checked. |
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? |
What is stopping the repo owners from merging this PR? |
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? |
Can this be merged now? What is taking so long? @arthanson |
Merge please |
Superseded by #616 |
Good news everybody! @jeffgdotorg merged @btriller's superior solution #616 ! We can finally upgrade Netbox! |
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.