Skip to content

Conversation

@ezr-ondrej
Copy link
Member

@ezr-ondrej ezr-ondrej commented Dec 10, 2025

Just small clean-up for #3264

Summary by Sourcery

Harden Clowder-based configuration of the kessel-relations API endpoint.

Bug Fixes:

  • Validate that a Clowder configuration is loaded before using it to configure dependencies.
  • Fail fast with clear errors when the kessel-relations API endpoint is missing from the Clowder dependency configuration.
  • Derive the kessel-relations API endpoint from the configured hostname and port instead of assuming a fixed port.

@ezr-ondrej ezr-ondrej requested a review from a team as a code owner December 10, 2025 07:26
@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 10, 2025

Reviewer's guide (collapsed on small PRs)

Reviewer's Guide

Refines Clowder dependency loading for the kessel-relations service by validating configuration presence, safely looking up the dependency endpoint, and using the endpoint’s configured port instead of a hard-coded value.

Class diagram for config clowder_config changes

classDiagram
    class AppConfig {
        +bool is_clowder
        +string kessel_relations_api_endpoint
        +int metrics_port
        +clowder_config()
    }

    class app_common_python {
        +LoadedConfig LoadedConfig
        +dict DependencyEndpoints
    }

    class LoadedConfig {
        +int metricsPort
    }

    class DependencyEndpoint {
        +string hostname
        +int port
    }

    AppConfig --> LoadedConfig : uses
    AppConfig --> DependencyEndpoint : uses
    app_common_python --> LoadedConfig : provides
    app_common_python --> DependencyEndpoint : provides
Loading

Flow diagram for updated clowder_config dependency loading

flowchart TD
    A[Call clowder_config] --> B[Import app_common_python]
    B --> C[Set cfg to app_common_python.LoadedConfig]
    C --> D{cfg is None?}
    D -->|Yes| E[Raise ValueError: Clowder is enabled, but no configuration was loaded]
    D -->|No| F[Get kessel_relations_dep from app_common_python.DependencyEndpoints]
    F --> G{kessel_relations_dep is None?}
    G -->|Yes| H[Raise ValueError: Could not find kessel-relations endpoint in Clowder configuration]
    G -->|No| I[Set kessel_relations_api_endpoint to hostname:port from kessel_relations_dep]
    I --> J[Set is_clowder to True]
    J --> K[Set metrics_port to cfg.metricsPort]
    K --> L[Return from clowder_config]
Loading

File-Level Changes

Change Details Files
Harden Clowder configuration handling and kessel-relations endpoint resolution.
  • Add an explicit error when Clowder is enabled but no configuration is loaded
  • Look up the kessel-relations api endpoint via app_common_python.DependencyEndpoints using safe .get access
  • Raise a clear error if the kessel-relations endpoint is missing from the Clowder configuration
  • Build the kessel-relations API endpoint using the dependency’s hostname and configured port instead of a hard-coded port
app/config.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

app/config.py Outdated
Comment on lines 48 to 52
kessel_relations_dep = app_common_python.DependencyEndpoints.get("kessel-relations", {}).get("api", None)
if kessel_relations_dep is None:
raise ValueError("Could not find kessel-relations endpoint in Clowder configuration")

self.kessel_relations_api_endpoint = f"{kessel_relations_dep.hostname}:{kessel_relations_dep.port}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
kessel_relations_dep = app_common_python.DependencyEndpoints.get("kessel-relations", {}).get("api", None)
if kessel_relations_dep is None:
raise ValueError("Could not find kessel-relations endpoint in Clowder configuration")
self.kessel_relations_api_endpoint = f"{kessel_relations_dep.hostname}:{kessel_relations_dep.port}"
kessel_inventory_dep = app_common_python.DependencyEndpoints.get("kessel-inventory", {}).get("api", None)
if kessel_inventory_dep is None:
raise ValueError("Could not find kessel-inventory endpoint in Clowder configuration")
self.kessel_inventory_api_endpoint = f"{kessel_inventory_dep.hostname}:{kessel_inventory_dep.port}"

Copy link
Member Author

Choose a reason for hiding this comment

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

Rebased and applied the new dependency

@ezr-ondrej ezr-ondrej force-pushed the fix_dependency_loading branch from 84fb8bb to 70da5dc Compare December 10, 2025 16:43
Copy link
Contributor

@fstavela fstavela left a comment

Choose a reason for hiding this comment

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

I tested this in ephemeral with enabled Kessel Phase 1 and got HTTP 500 errors on API requests. This is from the inventory logs:

grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
status = StatusCode.UNAVAILABLE
details = "failed to connect to all addresses; last error: UNKNOWN: ipv4:172.30.67.68:8000: Trying to connect an http1.x server (HTTP status 301)"
debug_error_string = "UNKNOWN:Error received from peer {grpc_message:"failed to connect to all addresses; last error: UNKNOWN: ipv4:172.30.67.68:8000: Trying to connect an http1.x server (HTTP status 301)", grpc_status:14}"

@kruai
Copy link
Collaborator

kruai commented Dec 11, 2025

I tested this in ephemeral with enabled Kessel Phase 1 and got HTTP 500 errors on API requests. This is from the inventory logs:

grpc._channel._MultiThreadedRendezvous: <_MultiThreadedRendezvous of RPC that terminated with:
status = StatusCode.UNAVAILABLE
details = "failed to connect to all addresses; last error: UNKNOWN: ipv4:172.30.67.68:8000: Trying to connect an http1.x server (HTTP status 301)"
debug_error_string = "UNKNOWN:Error received from peer {grpc_message:"failed to connect to all addresses; last error: UNKNOWN: ipv4:172.30.67.68:8000: Trying to connect an http1.x server (HTTP status 301)", grpc_status:14}"

I don't think their Clowder config has the grpc port yet. 8000 is not correct; kessel-inventory is using 9000 in all envs right now. We probably need to keep hardcoding that value until they get that configured

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

Successfully merging this pull request may close these issues.

3 participants