Conversation
WalkthroughThe changes standardize and enhance metadata handling across multiple components by introducing and integrating the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant ClientConfigCreator
participant ObjectMeta
User->>CLI: Execute "create-config" with alias, --namespace, --name
CLI->>ClientConfigCreator: Pass alias, namespace, and name
ClientConfigCreator->>ObjectMeta: Instantiate metadata (namespace, name)
ObjectMeta-->>ClientConfigCreator: Return metadata instance
ClientConfigCreator->>CLI: Return complete client configuration
CLI->>User: Confirm client configuration creation
sequenceDiagram
participant User
participant CLI
participant ExporterConfigCreator
participant ObjectMeta
User->>CLI: Execute "create-config" for exporter with alias, --namespace, --name, endpoint, token
CLI->>ExporterConfigCreator: Pass alias, namespace, name, endpoint, token
ExporterConfigCreator->>ObjectMeta: Instantiate metadata (namespace, name)
ObjectMeta-->>ExporterConfigCreator: Return metadata instance
ExporterConfigCreator->>CLI: Return complete exporter configuration
CLI->>User: Confirm exporter configuration creation
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (2)
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/jumpstarter/jumpstarter/config/client_config_test.py (1)
91-115: Fix inconsistent name assertion intest_client_config_from_file.The test asserts
config.name == f.name.split("/")[-1]but also assertsconfig.metadata.name == "testclient". This seems inconsistent as the name should be consistent across both fields.Apply this diff to fix the inconsistency:
- assert config.name == f.name.split("/")[-1] + assert config.name == "testclient"
🧹 Nitpick comments (8)
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (1)
10-11: Consider adding validation for namespace and name.While the options are correctly added, consider adding validation to ensure the namespace and name follow Kubernetes naming conventions (e.g., lowercase alphanumeric characters, '-' allowed, start/end with alphanumeric).
@click.command("create-config") -@click.option("--namespace", prompt=True) -@click.option("--name", prompt=True) +@click.option( + "--namespace", + prompt=True, + callback=lambda ctx, param, value: value.lower() if value else None, + help="Kubernetes-style namespace (lowercase alphanumeric and '-')" +) +@click.option( + "--name", + prompt=True, + callback=lambda ctx, param, value: value.lower() if value else None, + help="Kubernetes-style name (lowercase alphanumeric and '-')" +)Also applies to: 15-15
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (1)
17-28: Fix typo in namespace prompt message.There's a typo in the namespace prompt message: "nespace" should be "namespace".
- prompt="Enter a valid Jumpstarter client nespace", + prompt="Enter a valid Jumpstarter client namespace",packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)
108-111: Consider adding null checks for metadata fields.While the implementation is correct, it would be more robust to add null checks for
client.metadata.namespaceandclient.metadata.nameto handle edge cases gracefully.metadata=ObjectMeta( - namespace=client.metadata.namespace, - name=client.metadata.name, + namespace=getattr(client.metadata, 'namespace', 'default'), + name=getattr(client.metadata, 'name', name), ),packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py (1)
115-118: Consider adding null checks for metadata fields.Similar to the client configuration, it would be more robust to add null checks for
exporter.metadata.namespaceandexporter.metadata.nameto handle edge cases gracefully.metadata=ObjectMeta( - namespace=exporter.metadata.namespace, - name=exporter.metadata.name, + namespace=getattr(exporter.metadata, 'namespace', 'default'), + name=getattr(exporter.metadata, 'name', name), ),packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (2)
36-36: Consider usingObjectMetafor consistency.The PR introduces
ObjectMetato standardize metadata handling, butCLIENT_OBJECTstill usesV1ObjectMeta. Consider usingObjectMetafor consistency.Apply this diff:
- metadata=V1ObjectMeta(namespace="default", name=CLIENT_NAME, creation_timestamp="2024-01-01T21:00:00Z"), + metadata=ObjectMeta(namespace="default", name=CLIENT_NAME),
122-122: Consider usingObjectMetafor consistency.Similar to
CLIENT_OBJECT,EXPORTER_OBJECTstill usesV1ObjectMeta. Consider usingObjectMetafor consistency.Apply this diff:
- metadata=V1ObjectMeta(namespace="default", name=EXPORTER_NAME, creation_timestamp="2024-01-01T21:00:00Z"), + metadata=ObjectMeta(namespace="default", name=EXPORTER_NAME),packages/jumpstarter/jumpstarter/config/client.py (1)
146-149: Consider validating required metadata fields.The metadata initialization from environment variables should validate that required fields are present.
Consider adding validation before initializing metadata:
+ # Validate required metadata fields + namespace = os.environ.get(JMP_NAMESPACE) + name = os.environ.get(JMP_NAME) + if not namespace or not name: + raise ValueError("Both namespace and name are required in metadata") metadata=ObjectMeta( - namespace=os.environ.get(JMP_NAMESPACE), - name=os.environ.get(JMP_NAME), + namespace=namespace, + name=name, ),packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (1)
138-138: Consider usingObjectMetafor consistency.Similar to the issues in
create_test.py,EXPORTER_OBJECTstill usesV1ObjectMeta. Consider usingObjectMetafor consistency.Apply this diff:
- metadata=V1ObjectMeta(namespace="default", name=EXPORTER_NAME, creation_timestamp="2024-01-01T21:00:00Z"), + metadata=ObjectMeta(namespace="default", name=EXPORTER_NAME),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py(3 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py(3 hunks)packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py(3 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py(3 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py(1 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py(2 hunks)packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py(1 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py(2 hunks)packages/jumpstarter-kubernetes/jumpstarter_kubernetes/exporters.py(2 hunks)packages/jumpstarter/jumpstarter/config/__init__.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(3 hunks)packages/jumpstarter/jumpstarter/config/client_config_test.py(14 hunks)packages/jumpstarter/jumpstarter/config/common.py(1 hunks)packages/jumpstarter/jumpstarter/config/env.py(1 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(2 hunks)packages/jumpstarter/jumpstarter/config/exporter_test.py(5 hunks)packages/jumpstarter/jumpstarter/config/user_config_test.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (22)
packages/jumpstarter/jumpstarter/config/common.py (1)
3-11: LGTM! Well-structured metadata class.The
ObjectMetaclass is well-designed, using Pydantic for data validation and following Python naming conventions.packages/jumpstarter/jumpstarter/config/env.py (1)
3-4: LGTM! Environment variables follow consistent naming.The new environment variables
JMP_NAMESPACEandJMP_NAMEfollow the existing naming convention and are appropriately placed.packages/jumpstarter/jumpstarter/config/__init__.py (1)
5-5: LGTM! ObjectMeta properly exposed.The
ObjectMetaclass is correctly imported and exposed in the module's public interface.Also applies to: 18-18
packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_config.py (2)
4-4: LGTM! Import statement is correct.The import statement correctly includes
ObjectMetaalong withExporterConfigV1Alpha1.
24-29: LGTM! Configuration object correctly initialized.The
ExporterConfigV1Alpha1instance is properly created with theObjectMetacontaining namespace and name.packages/jumpstarter-cli-exporter/jumpstarter_cli_exporter/exporter_test.py (2)
19-31: LGTM! Test case updated to include metadata parameters.The non-interactive test case correctly validates the new metadata parameters (
--namespaceand--name).
54-56: LGTM! Interactive test case updated to include metadata parameters.The interactive test case correctly validates the new metadata parameters by providing namespace and name in the input string.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_test.py (2)
21-34: LGTM! Test case updated to include metadata parameters.The non-interactive test case correctly validates the new metadata parameters (
--namespaceand--name).
62-62: LGTM! Interactive test case updated to include metadata parameters.The interactive test case correctly validates the new metadata parameters by providing namespace and name in the input string.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (2)
6-6: LGTM! Import ObjectMeta from common module.The import statement correctly includes the new ObjectMeta class.
54-73: LGTM! Function signature and config creation updated.The changes improve clarity by:
- Renaming "name" parameter to "alias"
- Adding namespace and name parameters
- Using ObjectMeta in config creation
packages/jumpstarter/jumpstarter/config/exporter.py (2)
12-12: LGTM! Import ObjectMeta from common module.The import statement correctly includes the new ObjectMeta class.
39-39: LGTM! Add metadata field to ExporterConfigV1Alpha1.The metadata field is correctly added to the ExporterConfigV1Alpha1 class using the ObjectMeta type.
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py (1)
16-16: LGTM! Consistent metadata handling.The changes correctly integrate ObjectMeta into client and exporter configurations, maintaining consistency across the codebase.
Also applies to: 27-27, 34-34, 91-91
packages/jumpstarter/jumpstarter/config/exporter_test.py (1)
8-8: LGTM! Comprehensive test coverage for metadata.The test cases properly validate the metadata integration in both code and YAML configurations, ensuring consistent behavior across different usage scenarios.
Also applies to: 20-20, 43-43, 69-71, 110-110
packages/jumpstarter/jumpstarter/config/client.py (2)
12-13: LGTM!The imports are correctly organized and include the necessary components for metadata handling.
44-44: LGTM!The
metadatafield is correctly added with proper typing.packages/jumpstarter/jumpstarter/config/user_config_test.py (1)
10-10: LGTM!The metadata fields are consistently updated to use
ObjectMetaacross all test cases.Also applies to: 36-37, 68-69, 95-96, 198-199, 239-240, 251-252, 279-280
packages/jumpstarter/jumpstarter/config/client_config_test.py (4)
10-11: LGTM! Import changes are well-organized.The imports are correctly updated to include the new
ObjectMetaclass and environment variables.
21-36: LGTM! Test functions are consistently updated.The test functions are systematically updated to include metadata environment variables and assertions while maintaining existing test coverage.
Also applies to: 43-58, 60-75, 78-89
197-229: LGTM! YAML configurations are consistently updated.The test functions correctly include metadata in YAML configurations and maintain proper structure.
Also applies to: 232-262, 265-293
169-189: LGTM! Mock objects are correctly updated.The mock objects are properly updated to include metadata while maintaining the existing structure.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/config/grpc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/jumpstarter/jumpstarter/config/exporter.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/jumpstarter/jumpstarter/config/grpc.py (1)
6-6: Add docstring to improve code documentation.Please add a docstring to document the purpose, parameters, and return value of the function.
Add this docstring:
def call_credentials(kind: str, metadata: ObjectMeta, token: str): + """Create composite gRPC call credentials with metadata and access token. + + Args: + kind (str): The kind of resource being accessed + metadata (ObjectMeta): Object metadata containing namespace and name + token (str): Access token for authentication + + Returns: + grpc.CallCredentials: Composite call credentials for gRPC calls + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/jumpstarter/jumpstarter/config/client.py(4 hunks)packages/jumpstarter/jumpstarter/config/exporter.py(3 hunks)packages/jumpstarter/jumpstarter/config/grpc.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/config/exporter.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: pytest-matrix (3.13)
- GitHub Check: pytest-matrix (3.12)
- GitHub Check: e2e
- GitHub Check: pytest-matrix (3.11)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (1)
packages/jumpstarter/jumpstarter/config/grpc.py (1)
1-5: LGTM!The imports are appropriate and the module structure follows Python conventions.
|
Makes sense, please add new env variables and config format changes to documentation and update documentation (where the cli docs aren't too rusty, that is being updated by @kirkbrauer in #240 ) |
2 participants
Summary by CodeRabbit
New Features
Refactor
Tests
Related to: jumpstarter-dev/jumpstarter#92