Skip to content

Conversation

@wvandeun
Copy link
Contributor

@wvandeun wvandeun commented Oct 14, 2025

Add the tls_ca_file and tls_insecure configuration settings for infrahubctl.
This allows you to define these configuration settings in the infrahubctl configuration file.

It was already possible to configure these settings through environment variables.

Summary by CodeRabbit

  • New Features

    • Added TLS options to configure a custom CA certificate file and optionally disable certificate verification.
    • Available via environment variables (INFRAHUB_TLS_CA_FILE, INFRAHUB_TLS_INSECURE) and corresponding configuration file entries.
    • Client now respects these settings when establishing connections.
  • Documentation

    • Updated environment variables table and configuration examples to include the new TLS options and defaults.

@wvandeun wvandeun requested a review from a team October 14, 2025 21:09
@wvandeun wvandeun self-assigned this Oct 14, 2025
@github-actions github-actions bot added the type/documentation Improvements or additions to documentation label Oct 14, 2025
@coderabbitai
Copy link

coderabbitai bot commented Oct 14, 2025

Walkthrough

This change introduces two TLS-related configuration settings across the SDK and documentation. In infrahub_sdk/ctl/config.py, the Settings class gains tls_insecure: bool (default false) and tls_ca_file: str | None (default None). In infrahub_sdk/ctl/client.py, the client configuration now includes tls_ca_file and tls_insecure from config.SETTINGS.active when present. Documentation updates add environment variables INFRAHUB_TLS_CA_FILE and INFRAHUB_TLS_INSECURE and reflect these fields in the infrahubctl.toml example. No exported function or method signatures were modified.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title clearly and concisely describes the primary change by specifying the addition of the two TLS-related configuration settings tls_ca_file and tls_insecure to infrahubctl, matching both the code and documentation updates. It uses precise terminology and directly reflects the pull request’s main objective without unnecessary detail.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch wvd-20251014-ctl-tls-config

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 33.33333% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
infrahub_sdk/ctl/client.py 0.00% 2 Missing and 2 partials ⚠️
@@            Coverage Diff             @@
##           stable     #580      +/-   ##
==========================================
- Coverage   76.03%   75.72%   -0.31%     
==========================================
  Files         101      100       -1     
  Lines        9642     8902     -740     
  Branches     2020     1754     -266     
==========================================
- Hits         7331     6741     -590     
+ Misses       1769     1679      -90     
+ Partials      542      482      -60     
Flag Coverage Δ
integration-tests 34.67% <33.33%> (-0.05%) ⬇️
python-3.10 48.29% <33.33%> (-0.74%) ⬇️
python-3.11 48.29% <33.33%> (-0.73%) ⬇️
python-3.12 48.27% <33.33%> (-0.73%) ⬇️
python-3.13 48.27% <33.33%> (-0.71%) ⬇️
python-3.9 46.91% <33.33%> (-0.81%) ⬇️
python-filler-3.12 24.99% <33.33%> (-0.34%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
infrahub_sdk/ctl/config.py 66.03% <100.00%> (-3.41%) ⬇️
infrahub_sdk/ctl/client.py 46.15% <0.00%> (-8.40%) ⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a 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 (1)
docs/docs/infrahubctl/infrahubctl.mdx (1)

44-46: Consider documenting data types in the example.

While the TOML example is syntactically correct, users might benefit from comments indicating that tls_insecure is a boolean and tls_ca_file is a file path string.

Apply this diff to add clarifying comments:

 server_address="http://localhost:8000"
 api_token="06438eb2-8019-4776-878c-0941b1f1d1ec"
 default_branch="main"
+# Path to CA certificate file in PEM format (optional)
 tls_ca_file="/path/to/ca.crt"
+# Disable TLS certificate verification - useful for self-signed certs (boolean, default: false)
 tls_insecure=true
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8f58b5 and 9d54d32.

📒 Files selected for processing (3)
  • docs/docs/infrahubctl/infrahubctl.mdx (2 hunks)
  • infrahub_sdk/ctl/client.py (1 hunks)
  • infrahub_sdk/ctl/config.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
docs/**/*.{md,mdx}

📄 CodeRabbit inference engine (CLAUDE.md)

docs/**/*.{md,mdx}: Follow the Diataxis framework and classify docs as Tutorials, How-to guides, Explanation, or Reference
Structure How-to guides with required frontmatter and sections: introduction, prerequisites, step-by-step steps, validation, related resources
Structure Topics (Explanation) docs with introduction, concepts & definitions, background & context, architecture & design, connections, further reading
Use a professional, concise, informative tone with consistent structure across documents
Use proper language tags on all code blocks
Include both async and sync examples where applicable using the Tabs component
Add validation steps to guides to confirm success and expected outputs
Use callouts for warnings, tips, and important notes
Define new terms on first use and use domain-relevant terminology consistent with Infrahub’s model/UI
Conform to markdown style rules in .markdownlint.yaml and Vale styles in .vale/styles/

Files:

  • docs/docs/infrahubctl/infrahubctl.mdx
**/*.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/client.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
  • infrahub_sdk/ctl/client.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 : Environment variables for configuration must use the INFRAHUB_ prefix

Applied to files:

  • docs/docs/infrahubctl/infrahubctl.mdx
🧬 Code graph analysis (1)
infrahub_sdk/ctl/client.py (1)
infrahub_sdk/ctl/config.py (1)
  • active (44-49)
🪛 LanguageTool
docs/docs/infrahubctl/infrahubctl.mdx

[grammar] ~34-~34: There might be a mistake here.
Context: .../path/to/ca.crt | | INFRAHUB_TLS_INSECURE | true ...

(QB_NEW_EN)

⏰ 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.10)
  • GitHub Check: unit-tests (3.9)
  • GitHub Check: unit-tests (3.13)
  • GitHub Check: unit-tests (3.11)
  • GitHub Check: unit-tests (3.12)
  • GitHub Check: integration-tests-latest-infrahub
🔇 Additional comments (3)
docs/docs/infrahubctl/infrahubctl.mdx (1)

34-35: LGTM! Environment variable documentation is correct.

The new TLS-related environment variables follow the required INFRAHUB_ prefix and match the Settings class implementation. The example values are clear and appropriate.

Note: The static analysis grammar warning is a false positive—the markdown table is correctly formatted.

infrahub_sdk/ctl/client.py (1)

62-66: LGTM! TLS configuration integration follows existing patterns.

The conditional logic for both tls_ca_file and tls_insecure is consistent with how api_token is handled (lines 59-60), only passing values when they differ from defaults. This is the correct approach.

infrahub_sdk/ctl/config.py (1)

24-31: LGTM! TLS settings implementation is secure and well-documented.

The field definitions are correct with appropriate types, secure defaults, and clear descriptions. The environment variable names will correctly use the INFRAHUB_ prefix as required.

Based on learnings: Environment variables must use the INFRAHUB_ prefix, which is correctly configured here via env_prefix="INFRAHUB_" on line 20.

Copy link
Contributor

@ogenstad ogenstad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I'd change the stuff in the docs.

Does this mean that people are using infrahubctl.toml files?

I think we can merge this change for now but that we should get rid of the config class for Infrahubctl. If we want to provide a file the expected format should be a 1-1 match against what the SDK itself supports so that we don't have to add specific entries like this and include the if-statement logic for setting up the config of the client within the SDK.

| `INFRAHUB_API_TOKEN` | `06438eb2-8019-4776-878c-0941b1f1d1ec` |
| `INFRAHUB_DEFAULT_BRANCH` | main |
| `INFRAHUB_TLS_CA_FILE` | /path/to/ca.crt |
| `INFRAHUB_TLS_INSECURE` | true |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be an exact replica of what we support in the SDK as we reuse the existing environment variables. As such I think that instead of adding options here we can have the INFRAHUB_ADDRESS one as an example and then link to the config page for all of the SDK environment variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants