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

Include the pynautobot version in the User-Agent header by default #205

Closed
nrnvgh opened this issue Jul 3, 2024 · 7 comments
Closed

Include the pynautobot version in the User-Agent header by default #205

nrnvgh opened this issue Jul 3, 2024 · 7 comments

Comments

@nrnvgh
Copy link
Contributor

nrnvgh commented Jul 3, 2024

We ran into a case recently where it would have been useful to have pynautobot send its version in a user-agent header (or similar) by default; something like: python-pynautobot/1.5.2. For example, this would allow us to reach out to specific users and let them know they can/should upgrade to a more recent version. I'm thinking a patch like this:

--- a/pynautobot/core/api.py
+++ b/pynautobot/core/api.py
@@ -19,6 +19,7 @@ import requests
 from requests.adapters import HTTPAdapter
 from urllib3 import Retry

+import pynautobot
 from pynautobot.core.query import Request
 from pynautobot.core.app import App, PluginsApp
 from pynautobot.core.graphql import GraphQLQuery
@@ -83,6 +84,9 @@ class Api(object):
         self.base_url = base_url
         self.http_session = requests.Session()
         self.http_session.verify = verify
+        self.http_session.headers.update(
+            {"User-Agent": f"python-pynautobot/{pynautobot.__version__}"}
+        )
         if retries:
             _adapter = HTTPAdapter(
                 max_retries=Retry(

...which applies cleanly against both the ltm-1.6 and develop branches. Thoughts? I'm happy to fork and submit PRs for either or both branches if this works for you. If it does, would you accept it into the LTM branch or is this purely a 2.x thing? It's obviously not a bugfix, but as we're still on nautobot 1.6, I gotta ask. I could also extend the header to include the python-requests version as well if that would be useful.

@joewesch
Copy link
Contributor

joewesch commented Jul 3, 2024

This looks like a duplicate of #199. It was noted in that issue that this is already possible by setting the header after instantiation, but I can understand the desire to not need an extra step.

If it does, would you accept it into the LTM branch or is this purely a 2.x thing? It's obviously not a bugfix, but as we're still on nautobot 1.6, I gotta ask.

Yes, that seems like an acceptable low-risk change to me if you're willing to do the leg work.

@nrnvgh
Copy link
Contributor Author

nrnvgh commented Jul 3, 2024

@nrnvgh
Copy link
Contributor Author

nrnvgh commented Jul 4, 2024

This looks like a duplicate of #199. It was noted in that issue that this is already possible by setting the header after instantiation, but I can understand the desire to not need an extra step.

Also, in our case, we have a large (and unknown. and ever-changing.) number of folks who are writing pynautobot-enabled scripts to access our data, and asking everyone to update their code to send the pynautobot version in the useragent is... impractical. =)

@nrnvgh
Copy link
Contributor Author

nrnvgh commented Jul 5, 2024

I see the failure in the PR #206 ; is that a 'me' problem or a 'pipeline' problem? The test harness passes locally for me, modulo a DeprecationWarning for something outside of the pynautobot tree entirely.

@nrnvgh
Copy link
Contributor Author

nrnvgh commented Jul 5, 2024

I dug into this and, with a help from Slack, was ultimately able to trace this to an issue where __version__ doesn't get set in pynautobot/__init__.py when running under Docker. Under docker, package information for pynautobot is not available (the package isn't installed). Per suggestion from @jvanderaa, I've updated __init__.py in both my PR's so that if the package isn't found, set __version__ to something informative.

@nrnvgh
Copy link
Contributor Author

nrnvgh commented Aug 1, 2024

Both PRs have been merged; AFAIC this can be closed, with my thanks.

@joewesch
Copy link
Contributor

joewesch commented Aug 2, 2024

Thanks!

@joewesch joewesch closed this as completed Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants