Skip to content

fix: add labels to NodeNotFoundError for clearer messaging #1030

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.

Sign up for GitHub

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

Open
DharmaBytesX wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

fix: add labels to NodeNotFoundError for clearer messaging #1030

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
095371a
fix: add labels to NodeNotFoundError for clearer messaging
DharmaBytesX May 17, 2026
1ab16ae
fix: make NodeNotFoundError parameters required
DharmaBytesX May 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion infrahub_sdk/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,16 @@ def __init__(
super().__init__(self.message)

def __str__(self) -> str:
detail_parts = []
if self.branch_name:
detail_parts.append(f"Branch: {self.branch_name}")
if self.node_type:
detail_parts.append(f"Kind: {self.node_type}")

@ogenstad ogenstad May 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think we should have if-statements within the __str__() method of classes in general. I think the problem here is really that the parameters to the __init__() method are optional instead of being required. We should probably look at the places where we raise this error and check if we are always sending in all fields (or if we have enough information to do so) and then update the init method to require there parameters.

@DharmaBytesX DharmaBytesX May 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the review. I've reworked the change along the lines you suggested: branch_name, node_type and identifier are now required keyword-only parameters of NodeNotFoundError.__init__, and __str__ no longer has any conditional logic.

I audited the call sites and updated them to always pass the three fields:

  • infrahub_sdk/client.py was already passing all three.
  • infrahub_sdk/ctl/object/utils.py now resolves the branch to client.default_branch when the caller did not provide one.
  • infrahub_sdk/file_handler.py: handle_response and handle_error_response now take branch and node_id, plumbed from FileHandler.download / _stream_to_file in both the async and sync paths.
  • infrahub_sdk/store.py: passes self.branch_name, and falls back to "unknown" only on the internal lookup paths where the caller genuinely never specified a kind.

Also added tests/unit/sdk/test_exceptions.py covering the new contract (keyword-only, all three required, rendered format).

detail_parts.append(f"Identifier: {self.identifier}")
detail = " | ".join(detail_parts)
return f"""
{self.message}
{self.branch_name} | {self.node_type} | {self.identifier}
{detail}
"""


Expand Down