IFC-1811: Replace toml package with tomllib and tomli optionally #551
Conversation
WalkthroughReplaces direct Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
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-sdk-python with
|
| Latest commit: |
1b7e6a7
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f4da0ad3.infrahub-sdk-python.pages.dev |
| Branch Preview URL: | https://sb-20250917-replace-toml-ifc.infrahub-sdk-python.pages.dev |
github-actions
bot
added
group/ci
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
239-244: Remove redundant tomli install and unused invoke.Poetry will install tomli for <3.11 per pyproject; system pip won’t affect the poetry venv. Also “invoke” isn’t used in this job.
Apply:
- pipx install poetry==${{ needs.prepare-environment.outputs.POETRY_VERSION }} --python python${{ matrix.python-version }} - poetry config virtualenvs.create true --local - pip install invoke codecov + pipx install poetry==${{ needs.prepare-environment.outputs.POETRY_VERSION }} --python python${{ matrix.python-version }} + poetry config virtualenvs.create true --local + pip install codecov @@ - - name: "Install tomli for Python < 3.11" - if: matrix.python-version == '3.9' || matrix.python-version == '3.10' - run: | - pip install tomli + # tomli installed via poetry for <3.11 per pyproject; no extra step needed
296-296: Integration tests: drop unused invoke from system pip step.- pipx install poetry==${{ needs.prepare-environment.outputs.POETRY_VERSION }} + pipx install poetry==${{ needs.prepare-environment.outputs.POETRY_VERSION }} poetry config virtualenvs.create true --local - pip install invoke codecov + pip install codecov
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.yml(5 hunks)docs/docusaurus.config.ts(1 hunks)infrahub_sdk/ctl/config.py(2 hunks)pyproject.toml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
When implementing Infrahub checks, subclass InfrahubCheck and override validate(data); do not implement or rely on a check() method
Files:
infrahub_sdk/ctl/config.py
infrahub_sdk/ctl/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
infrahub_sdk/ctl/**/*.py: Build CLI commands with Typer
Organize and keep all CLI commands within infrahub_sdk/ctl/
Files:
infrahub_sdk/ctl/config.py
🧠 Learnings (1)
📚 Learning: 2025-08-24T13:35:12.998Z
Learnt from: CR
PR: opsmill/infrahub-sdk-python#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-24T13:35:12.998Z
Learning: Applies to infrahub_sdk/config.py : Use a Pydantic-based Config class with environment variable support for configuration
Applied to files:
infrahub_sdk/ctl/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration-tests-latest-infrahub
🔇 Additional comments (4)
pyproject.toml (2)
72-72: Extras cleaned up (toml removed) — LGTM.
48-48: Good: correct stdlib-first fallback via tomli for <3.11 — no runtime "toml" imports found.
Searched tracked .py files (ripgrep + git ls-files + find) for import/from/toml and toml.* usages; no matches.infrahub_sdk/ctl/config.py (1)
11-15: Compat import pattern is correct.tomllib with tomli fallback is implemented cleanly and namespaced consistently.
.github/workflows/ci.yml (1)
149-149: Docs job: fine to drop toml install; keep.
| if config_file.is_file(): | ||
| config_string = config_file.read_text(encoding="utf-8") | ||
| config_tmp = toml.loads(config_string) | ||
| config_tmp = tomllib.loads(config_string) |
There was a problem hiding this comment.
Handle TOML parse errors and fix doc typos.
Currently TOML decode errors bubble unhandled; load_and_exit only catches ValidationError. Catch tomllib.TOMLDecodeError (works for both tomllib and tomli alias) and show a clear message. Also fix typos and align parameter names; prefer DEFAULT_CONFIG_FILE constant in signatures.
Apply:
- def load(self, config_file: str | Path = "infrahubctl.toml", config_data: dict | None = None) -> None:
+ def load(self, config_file: str | Path = DEFAULT_CONFIG_FILE, config_data: dict | None = None) -> None:
@@
- if config_file.is_file():
- config_string = config_file.read_text(encoding="utf-8")
- config_tmp = tomllib.loads(config_string)
+ if config_file.is_file():
+ config_string = config_file.read_text(encoding="utf-8")
+ try:
+ config_tmp = tomllib.loads(config_string)
+ except tomllib.TOMLDecodeError as exc:
+ print(f"Configuration file parse error: {config_file} — {exc}") # consider typer.secho
+ raise typer.Abort()
@@
- def load_and_exit(self, config_file: str | Path = "infrahubctl.toml", config_data: dict | None = None) -> None:
+ def load_and_exit(self, config_file: str | Path = DEFAULT_CONFIG_FILE, config_data: dict | None = None) -> None:
@@
- """Calls load, but wraps it in a try except block.
+ """Calls load, but wraps it in a try/except block.
@@
- This is done to handle a ValidationErorr which is raised when settings are specified but invalid.
+ This handles a ValidationError when settings are specified but invalid.
@@
- config_file_name (str, optional): [description]. Defaults to "pyprojectctl.toml".
+ config_file (str | Path, optional): Path to TOML config. Defaults to "infrahubctl.toml".
@@
- except ValidationError as exc:
+ except ValidationError as exc:
print(f"Configuration not valid, found {len(exc.errors())} error(s)")
for error in exc.errors():
loc_str = [str(item) for item in error["loc"]]
print(f" {'/'.join(loc_str)} | {error['msg']} ({error['type']})")
raise typer.Abort()Optional: replace print with typer.echo/secho for CLI UX.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config_tmp = tomllib.loads(config_string) | |
| try: | |
| config_tmp = tomllib.loads(config_string) | |
| except tomllib.TOMLDecodeError as exc: | |
| print(f"Configuration file parse error: {config_file} — {exc}") # consider typer.secho | |
| raise typer.Abort() |
🤖 Prompt for AI Agents
In infrahub_sdk/ctl/config.py around line 66, the call to tomllib.loads can
raise tomllib.TOMLDecodeError which is currently unhandled; update load_and_exit
to catch tomllib.TOMLDecodeError (compatible with tomli alias), log or echo a
clear, user-facing message about invalid TOML (include the error message), and
exit similarly to ValidationError handling; also fix docstring/print typos and
align function parameter names to use the DEFAULT_CONFIG_FILE constant in
function signatures and docs, and optionally replace print calls with
typer.echo/secho for better CLI output.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## develop #551 +/- ##
===========================================
- Coverage 75.97% 75.88% -0.10%
===========================================
Files 100 100
Lines 9345 8887 -458
Branches 1835 1741 -94
===========================================
- Hits 7100 6744 -356
+ Misses 1754 1668 -86
+ Partials 491 475 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.pre-commit-config.yaml (2)
1-2: Add repo-wide hygiene keys (exclude; optional min version).Recommend a lightweight top-level exclude to skip vendor/build dirs; optionally pin a minimum pre-commit version.
Apply:
--- +minimum_pre_commit_version: "3.0.0" +exclude: '^(.git|.venv|.mypy_cache|.ruff_cache|dist|build|node_modules)/' repos:If you’re unsure about the minimum version supported in your CI/dev environments, confirm before adding it.
3-12: Broaden baseline safety hooksAdd low-noise, high-signal hooks to .pre-commit-config.yaml: debug-statements, detect-private-key, check-added-large-files, mixed-line-ending, check-json. Upstream latest stable tag is v6.0.0 (Aug 9, 2025) and these hook IDs are present in that release. (github.com)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/ci.yml(5 hunks).pre-commit-config.yaml(1 hunks)docs/docusaurus.config.ts(1 hunks)infrahub_sdk/ctl/config.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/docusaurus.config.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- infrahub_sdk/ctl/config.py
- .github/workflows/ci.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: unit-tests (3.13)
- GitHub Check: unit-tests (3.12)
- GitHub Check: integration-tests-latest-infrahub
- GitHub Check: unit-tests (3.10)
- GitHub Check: unit-tests (3.9)
- GitHub Check: unit-tests (3.11)
🔇 Additional comments (1)
.pre-commit-config.yaml (1)
14-22: Pin ruff-pre-commit rev to a concrete tag and align with pyproject
- pyproject.toml pins ruff = "0.11.0" (line ~63); .pre-commit-config.yaml uses rev: v0.11.9 (lines 14–22). Latest ruff-pre-commit tag is v0.13.0 (2025-09-10).
- Action: either keep rev v0.11.9 to match pyproject, or bump both pyproject (ruff) and .pre-commit-config.yaml rev to v0.13.0. Add --unsafe-fixes to the ruff args only if your policy permits. Ruff supports Python ≥3.7 (so 3.9/3.10 are supported).
There was a problem hiding this comment.
LGTM. I just added a minor comment regarding the optional aspect of the tomli package.
One additional thing that would be nice is that we're using towncrier to generate release notes. As part of this PR it would be good to run towncrier create and indicate that the change is for issue #528 (without the pound character). Then just write it as a housekeeping issue that we've removed "toml" as a requirement and added "tomli" for Python versions under 3.11.
pyproject.toml
Outdated
| netutils = "^1.0.0" | ||
| click = { version = "8.1.*", optional = true } | ||
| copier = { version = "^9.8.0", optional = true } | ||
| tomli = { version = ">=1.1.0", python = "<3.11" } |
There was a problem hiding this comment.
I think we still want tomli to be optional=True and included in the poetry.extras ctl section just like 'toml' was as we don't need to have it installed if someone is only installing the SDK and won't use the infrahubctl command.
changelog/528.housekeeping.md
Outdated
| @@ -0,0 +1 @@ | |||
| Replace toml package with tomllib and tomli optionally for when python version is less than 3.11 | |||
There was a problem hiding this comment.
We typically write Python with a capital "P".
2 participants
Description
Replace toml package with tomllib and tomli optionally when project python version is
<3.11as tomllib ships withpython 3.11Test
[x] - Run test
[x] - Update docs
[x] - Add towncrier message
Summary by CodeRabbit
New Features
Documentation
Chores
Refactor