Skip to content
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.

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

Rename ClientConfig.name to ClientConfig.alias #254

Merged
merged 3 commits into from
Feb 6, 2025
Merged

Conversation

NickCao
Copy link
Collaborator

@NickCao NickCao commented Feb 6, 2025

Summary by CodeRabbit

  • Refactor
    • Standardized client and exporter configuration naming from name to alias for improved clarity and consistency.
  • Chore
    • Updated the build configuration by removing an outdated testing target.
  • Tests
    • Adjusted test cases to align with the updated configuration naming convention, ensuring all references now use alias.

Copy link
Contributor

coderabbitai bot commented Feb 6, 2025

Walkthrough

The changes remove the test-jumpstarter phony target from the Makefile and update configuration handling across the codebase. Specifically, all references to the configuration property name have been replaced with alias in both implementation and test files. This update spans client and exporter configurations, deletion logic, serialization, and associated tests. The functional behavior remains unchanged while ensuring consistent naming conventions across components.

Changes

File(s) Change Summary
Makefile Removed phony target test-jumpstarter from the .PHONY declaration.
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/{create_test.py, delete.py, delete_test.py, import_res_test.py} Renamed the configuration parameter from name to alias in client/exporter objects and updated deletion condition to compare against alias.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py Updated function signatures and internal logic in create_client_config, set_next_client, and use_client_config to use alias instead of name.
packages/jumpstarter/jumpstarter/config/client.py Replaced the name attribute with alias in the ClientConfigV1Alpha1 class, updating method signatures and internal logic accordingly.
packages/jumpstarter/jumpstarter/config/client_config_test.py Adjusted test cases to assert the alias attribute instead of name.
packages/jumpstarter/jumpstarter/config/user.py Modified serialization to return alias for the current client instead of name.
packages/jumpstarter/jumpstarter/config/user_config_test.py Updated tests to validate user configuration changes by checking alias in place of name.
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py Changed parameter name from name to alias in get_client_config method for ClientConfigV1Alpha1 instantiation.

Suggested reviewers

  • mangelajo
  • kirkbrauer

Poem

In a burrow of code, I leap with delight,
Hopping over changes in the soft moonlight.
"Name" has become "alias" in our clever new song,
With tests and configs dancing smoothly along.
A carrot of progress for all to admire—
Coding like a rabbit, we never tire!
🥕🐇

Tip

🌐 Web search-backed reviews and chat
  • We have enabled web search-based reviews and chat for all users. This feature allows CodeRabbit to access the latest documentation and information on the web.
  • You can disable this feature by setting web_search: false in the knowledge_base settings.
  • Please share any feedback in the Discord discussion.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a4a01e and 44176de.

📒 Files selected for processing (1)
  • packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1 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 (1)
packages/jumpstarter-kubernetes/jumpstarter_kubernetes/clients.py (1)

107-107: LGTM! Parameter name change aligns with the PR objectives.

The change from name to alias in the ClientConfigV1Alpha1 instantiation is consistent with the broader effort to update the configuration property naming across the codebase.

✨ Finishing Touches
  • 📝 Generate Docstrings (Beta)

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@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

🔭 Outside diff range comments (4)
packages/jumpstarter/jumpstarter/config/user_config_test.py (1)

67-72: Update remaining test functions to use alias instead of name.

For consistency with the PR objective, update the client configurations in test_user_config_load_no_current_client and test_user_config_load_current_client_empty to use alias instead of name.

Apply this diff to update the remaining test functions:

-            name="testclient",
+            alias="testclient",

Also applies to: 94-99

packages/jumpstarter/jumpstarter/config/client_config_test.py (3)

91-108: Update test YAML content to use alias field.

While the assertion has been updated to check alias, the test YAML content still uses the old structure. This could lead to test failures.

Update the YAML content to match the new structure:

 CLIENT_CONFIG = """apiVersion: jumpstarter.dev/v1alpha1
 kind: ClientConfig
 metadata:
   namespace: default
   name: testclient
+alias: testclient
 endpoint: jumpstarter.my-lab.com:1443
 token: dGhpc2lzYXRva2VuLTEyMzQxMjM0MTIzNEyMzQtc2Rxd3Jxd2VycXdlcnF3ZXJxd2VyLTEyMzQxMjM0MTIz
 drivers:
   allow:
   - jumpstarter.drivers.*
   - vendorpackage.*
"""

198-213: Update test YAML content in save tests to use alias field.

The test YAML content in all save tests still uses the old structure and needs to be updated to include the alias field.

Update the YAML content in all save tests. Example for the first test:

 CLIENT_CONFIG = """apiVersion: jumpstarter.dev/v1alpha1
 kind: ClientConfig
 metadata:
   namespace: default
   name: testclient
+alias: testclient
 endpoint: jumpstarter.my-lab.com:1443
 tls:
   ca: ''
   insecure: false
 token: dGhpc2lzYXRva2VuLTEyMzQxMjM0MTIzNEyMzQtc2Rxd3Jxd2VycXdlcnF3ZXJxd2VyLTEyMzQxMjM0MTIz
 drivers:
   allow:
   - jumpstarter.drivers.*
   - vendorpackage.*
   unsafe: false
"""

Apply similar changes to the YAML content in test_client_config_save_explicit_path and test_client_config_save_unsafe_drivers.

Also applies to: 233-248, 266-279


305-316: Update test YAML content in list test to use alias field.

While the assertion has been updated to check alias, the test YAML content still uses the old structure.

Update the YAML content:

 CLIENT_CONFIG = """apiVersion: jumpstarter.dev/v1alpha1
 kind: ClientConfig
 metadata:
   namespace: default
   name: testclient
+alias: testclient
 endpoint: jumpstarter.my-lab.com:1443
 token: dGhpc2lzYXRva2VuLTEyMzQxMjM0MTIzNEyMzQtc2Rxd3Jxd2VycXdlcnF3ZXJxd2VyLTEyMzQxMjM0MTIz
 drivers:
   allow:
   - jumpstarter.drivers.*
   - vendorpackage.*
"""

Also applies to: 325-325

🧹 Nitpick comments (2)
packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py (1)

33-34: Rename parameter for consistency.

For consistency with the renaming from name to alias, consider renaming the function parameter as well.

-@click.argument("name", type=str, required=False, default=None)
+@click.argument("alias", type=str, required=False, default=None)

-async def delete_client(name: Optional[str], kubeconfig: Optional[str], context: Optional[str], namespace: str, delete: bool):
+async def delete_client(alias: Optional[str], kubeconfig: Optional[str], context: Optional[str], namespace: str, delete: bool):

Also applies to: 46-46, 57-57

packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (1)

83-83: Rename function parameters for consistency.

For consistency with the renaming from name to alias, consider renaming the function parameters in these functions.

-def set_next_client(name: str):
+def set_next_client(alias: str):

-def delete_client_config(name: str):
+def delete_client_config(alias: str):

-def use_client_config(name: str):
+def use_client_config(alias: str):

Also applies to: 99-101, 131-133

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 18994d0 and 5a4a01e.

📒 Files selected for processing (10)
  • Makefile (1 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (1 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete.py (1 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (1 hunks)
  • packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py (1 hunks)
  • packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (3 hunks)
  • packages/jumpstarter/jumpstarter/config/client.py (6 hunks)
  • packages/jumpstarter/jumpstarter/config/client_config_test.py (9 hunks)
  • packages/jumpstarter/jumpstarter/config/user.py (1 hunks)
  • packages/jumpstarter/jumpstarter/config/user_config_test.py (7 hunks)
✅ Files skipped from review due to trivial changes (1)
  • Makefile
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • 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 (19)
packages/jumpstarter/jumpstarter/config/user.py (1)

13-17: Verify backward compatibility with existing serialized configurations.

The change from v.name to v.alias might affect existing serialized configurations. Ensure there's a migration path for existing configurations.

packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (1)

67-73: LGTM! Configuration creation is correctly updated.

The implementation correctly uses the new alias field in the configuration creation.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/import_res_test.py (1)

25-38: LGTM! Test configurations are correctly updated.

The test configurations have been properly updated to use the new alias field.

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/create_test.py (2)

40-46: LGTM!

The changes correctly update the client configuration to use alias instead of name, maintaining consistency with the PR objective.

Also applies to: 48-54


125-130: LGTM!

The changes correctly update the exporter configuration to use alias instead of name, maintaining consistency with the PR objective.

Also applies to: 141-146

packages/jumpstarter/jumpstarter/config/client.py (6)

39-39: LGTM!

The class attribute has been correctly renamed from name to alias, maintaining the same default value and exclude flag.


127-127: LGTM!

The file path parsing has been correctly updated to set the alias attribute instead of name.


160-162: LGTM!

The method signature and docstring have been correctly updated to use alias instead of name.


165-167: LGTM!

The method signature and docstring have been correctly updated to use alias instead of name.


179-179: LGTM!

The path construction has been correctly updated to use alias instead of name.


190-192: LGTM!

The method signatures and docstrings have been correctly updated to use alias instead of name.

Also applies to: 212-214

packages/jumpstarter-cli-admin/jumpstarter_cli_admin/delete_test.py (2)

31-37: LGTM!

The client configuration has been correctly updated to use alias instead of name.


141-146: LGTM!

The exporter configuration has been correctly updated to use alias instead of name.

packages/jumpstarter/jumpstarter/config/user_config_test.py (4)

35-36: LGTM!

The client configuration and assertion have been correctly updated to use alias instead of name.

Also applies to: 48-48


197-198: LGTM!

The client configuration has been correctly updated to use alias instead of name.


238-239: LGTM!

The client configurations and assertion have been correctly updated to use alias instead of name.

Also applies to: 250-251, 263-263


278-279: LGTM!

The client configuration has been correctly updated to use alias instead of name.

packages/jumpstarter/jumpstarter/config/client_config_test.py (2)

29-29: LGTM! Environment-based configuration tests updated correctly.

The changes consistently replace name with alias across all environment-based configuration tests while maintaining comprehensive test coverage.

Also applies to: 51-51, 68-68


178-178: LGTM! Constructor calls and assertions updated correctly.

The changes consistently update all constructor calls and assertions to use alias.

Also applies to: 186-186, 215-215, 250-250, 281-281

Copy link
Member

@mangelajo mangelajo left a comment

Choose a reason for hiding this comment

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

Makes sense.... otherwise we have .name and .metadata.name

@NickCao NickCao merged commit 1025017 into main Feb 6, 2025
13 checks passed
@NickCao NickCao deleted the client-config-alias branch February 6, 2025 17:25
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.

2 participants