-
Notifications
You must be signed in to change notification settings - Fork 9
ipfabric: Add logic to fetch URL and token from environment variables. #98
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
Conversation
WalkthroughAdds environment-based fallbacks in the IP Fabric sync adapter: if base_url is absent in settings, it reads IPF_URL from the environment and stores it back; if auth is absent, it reads IPF_TOKEN and stores it back. Imports os to access environment variables. After attempting fallbacks, enforces configuration by raising ValueError("Both url and auth must be specified!") if either remains missing. Adjusts control flow in _create_ipfabric_client to populate settings from environment before client creation. No changes to public APIs. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying infrahub-sync with
|
| Latest commit: |
7c90fec
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://38b80744.infrahub-sync.pages.dev |
| Branch Preview URL: | https://may-202510-ipfabric-support.infrahub-sync.pages.dev |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
infrahub_sync/adapters/ipfabricsync.py (3)
52-54: Avoid storingNonewhen the environment variable is absent.The current implementation stores
Noneintosettings["base_url"]whenIPF_URLis not set in the environment. While this doesn't break functionality (the validation at line 61 catches it), it's unnecessary mutation of the settings dictionary.Consider this refactor to only update settings when a value is found:
-if not base_url: - base_url = os.environ.get("IPF_URL", None) - settings["base_url"] = base_url +if not base_url: + env_url = os.environ.get("IPF_URL") + if env_url: + base_url = env_url + settings["base_url"] = base_url
57-59: Avoid storingNonewhen the environment variable is absent.Same issue as with
base_url: storingNoneintosettings["auth"]whenIPF_TOKENis not set is unnecessary mutation. While functionally correct, it's cleaner to only update settings when a value is actually found.Apply this diff:
-if not auth: - auth = os.environ.get("IPF_TOKEN", None) - settings["auth"] = auth +if not auth: + env_token = os.environ.get("IPF_TOKEN") + if env_token: + auth = env_token + settings["auth"] = auth
52-59: Consider using constants for environment variable names.For better maintainability and to prevent typos, consider defining constants for the environment variable names at the module level:
IPF_URL_ENV = "IPF_URL" IPF_TOKEN_ENV = "IPF_TOKEN"Then reference them in the code:
env_url = os.environ.get(IPF_URL_ENV) # ... env_token = os.environ.get(IPF_TOKEN_ENV)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
infrahub_sync/adapters/ipfabricsync.py(2 hunks)
🔇 Additional comments (1)
infrahub_sync/adapters/ipfabricsync.py (1)
3-3: LGTM!The
osimport is appropriate for accessing environment variables.
Summary by CodeRabbit