fix service env issue and support specify service-name #56
Conversation
Summary of ChangesHello @JacksonMei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces enhanced flexibility for environment service creation by allowing users to define custom service names, which directly translates to Kubernetes resource names. It also includes robust validation for these custom names to adhere to Kubernetes standards. Furthermore, a critical fix for environment variable handling ensures that service configurations are correctly propagated across the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify a custom service name when creating a service, which is a great feature. The changes span across the Python client, Go backend, and documentation. My review focuses on improving the validation of the new service name, ensuring consistency in configuration keys, and simplifying some of the new logic. I've found a high-severity issue with the service name validation that could lead to invalid names being accepted by the CLI. I've also included suggestions to improve code style and documentation accuracy.
| if service_name: | ||
| import re | ||
| # Kubernetes DNS-1123 subdomain: lowercase alphanumeric, hyphens, dots; max 253 chars; must start/end with alphanumeric | ||
| dns_pattern = r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' | ||
| if not re.match(dns_pattern, service_name) or len(service_name) > 253: | ||
| console.print( | ||
| "[red]Error:[/red] Invalid service name. Service name must:\n" | ||
| " - Use only lowercase letters, numbers, hyphens, and dots\n" | ||
| " - Start and end with an alphanumeric character\n" | ||
| " - Be no longer than 253 characters\n" | ||
| f" - Example: 'my-service', 'app-v1', 'web-frontend-prod'" | ||
| ) | ||
| raise click.Abort() |
There was a problem hiding this comment.
This validation block for service_name has a couple of issues:
- Incorrect Validation: The validation does not correctly enforce Kubernetes DNS-1123 label standards for a service name. It allows dots (
.) and checks for a maximum length of 253 characters, while a service name (a single label) must not contain dots and be at most 63 characters long. This could lead to users providing invalid service names that will be rejected by the Kubernetes API. - Local Import: The
import restatement is inside the function. According to PEP 8, imports should be at the top of the file for better readability and to easily see module dependencies.
I've provided a code suggestion to fix the validation logic. Please also move import re to the top of the file.
| if service_name: | |
| import re | |
| # Kubernetes DNS-1123 subdomain: lowercase alphanumeric, hyphens, dots; max 253 chars; must start/end with alphanumeric | |
| dns_pattern = r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$' | |
| if not re.match(dns_pattern, service_name) or len(service_name) > 253: | |
| console.print( | |
| "[red]Error:[/red] Invalid service name. Service name must:\n" | |
| " - Use only lowercase letters, numbers, hyphens, and dots\n" | |
| " - Start and end with an alphanumeric character\n" | |
| " - Be no longer than 253 characters\n" | |
| f" - Example: 'my-service', 'app-v1', 'web-frontend-prod'" | |
| ) | |
| raise click.Abort() | |
| if service_name: | |
| # A Kubernetes service name must be a valid DNS-1123 label. | |
| # - must be at most 63 characters long | |
| # - must consist of lower case alphanumeric characters or '-' | |
| # - must start and end with an alphanumeric character | |
| dns_label_pattern = r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?$' | |
| if not re.match(dns_label_pattern, service_name) or len(service_name) > 63: | |
| console.print( | |
| "[red]Error:[/red] Invalid service name. A service name must be a valid DNS-1123 label, which means it:\n" | |
| " - Must be no longer than 63 characters\n" | |
| " - Must use only lowercase letters, numbers, and hyphens ('-')\n" | |
| " - Must start and end with an alphanumeric character\n" | |
| f" - Example: 'my-service', 'app-v1', 'web-frontend-prod'" | |
| ) | |
| raise click.Abort() |
| if req.ServiceName != "" { | ||
| backendEnv.DeployConfig["serviceName"] = req.ServiceName | ||
| } |
There was a problem hiding this comment.
The key serviceName (camelCase) is being used to store the custom service name in DeployConfig. However, this PR is also fixing other keys in DeployConfig to use snake_case (e.g., environment_variables). For consistency across the codebase, please use service_name (snake_case) as the key.
| if req.ServiceName != "" { | |
| backendEnv.DeployConfig["serviceName"] = req.ServiceName | |
| } | |
| if req.ServiceName != "" { | |
| backendEnv.DeployConfig["service_name"] = req.ServiceName | |
| } |
| // Generate service name: use custom serviceName from DeployConfig if provided, otherwise auto-generate | ||
| var serviceName string | ||
| if customServiceName, ok := aenvHubEnv.DeployConfig["serviceName"]; ok { | ||
| if customServiceNameStr, ok := customServiceName.(string); ok && customServiceNameStr != "" { | ||
| serviceName = customServiceNameStr | ||
| klog.Infof("Using custom service name: %s", serviceName) | ||
| } | ||
| } | ||
|
|
||
| // If no custom serviceName provided, auto-generate using envName and random suffix | ||
| if serviceName == "" { | ||
| serviceName = fmt.Sprintf("%s-svc-%s", aenvHubEnv.Name, RandString(6)) | ||
| klog.Infof("Auto-generated service name: %s", serviceName) | ||
| } |
There was a problem hiding this comment.
This block of code for determining the service name can be improved in two ways:
- Consistency: The key
serviceNameshould beservice_nameto be consistent with otherDeployConfigkeys (likeenvironment_variables) and the API request model. I've made a corresponding suggestion inapi-service/controller/env_service.go. - Readability: The logic can be refactored into a more concise
if/elsestructure to improve readability.
| // Generate service name: use custom serviceName from DeployConfig if provided, otherwise auto-generate | |
| var serviceName string | |
| if customServiceName, ok := aenvHubEnv.DeployConfig["serviceName"]; ok { | |
| if customServiceNameStr, ok := customServiceName.(string); ok && customServiceNameStr != "" { | |
| serviceName = customServiceNameStr | |
| klog.Infof("Using custom service name: %s", serviceName) | |
| } | |
| } | |
| // If no custom serviceName provided, auto-generate using envName and random suffix | |
| if serviceName == "" { | |
| serviceName = fmt.Sprintf("%s-svc-%s", aenvHubEnv.Name, RandString(6)) | |
| klog.Infof("Auto-generated service name: %s", serviceName) | |
| } | |
| // Generate service name: use custom serviceName from DeployConfig if provided, otherwise auto-generate | |
| var serviceName string | |
| if customName, ok := aenvHubEnv.DeployConfig["service_name"].(string); ok && customName != "" { | |
| serviceName = customName | |
| klog.Infof("Using custom service name: %s", serviceName) | |
| } else { | |
| serviceName = fmt.Sprintf("%s-svc-%s", aenvHubEnv.Name, RandString(6)) | |
| klog.Infof("Auto-generated service name: %s", serviceName) | |
| } |
| - **Service Name**: Custom service names must follow Kubernetes DNS naming conventions: | ||
| - Use only lowercase letters, numbers, hyphens, and dots | ||
| - Start and end with an alphanumeric character | ||
| - Be no longer than 253 characters | ||
| - Example: `my-service`, `app-v1`, `web-frontend-prod` | ||
| - If not specified, auto-generated as `{envName}-svc-{random}` (e.g., `myapp-svc-abc123`) |
There was a problem hiding this comment.
This documentation for service name constraints is incorrect and reflects the faulty validation logic in the CLI. A Kubernetes service name must be a DNS-1123 label, which does not allow dots and has a maximum length of 63 characters. The documentation should be updated to reflect the correct constraints.
| - **Service Name**: Custom service names must follow Kubernetes DNS naming conventions: | |
| - Use only lowercase letters, numbers, hyphens, and dots | |
| - Start and end with an alphanumeric character | |
| - Be no longer than 253 characters | |
| - Example: `my-service`, `app-v1`, `web-frontend-prod` | |
| - If not specified, auto-generated as `{envName}-svc-{random}` (e.g., `myapp-svc-abc123`) | |
| - **Service Name**: Custom service names must follow Kubernetes DNS-1123 label standards: | |
| - Must be no longer than 63 characters | |
| - Must use only lowercase letters, numbers, and hyphens ('-') | |
| - Must start and end with an alphanumeric character | |
| - Example: `my-service`, `app-v1`, `web-frontend-prod` | |
| - If not specified, auto-generated as `{envName}-svc-{random}` (e.g., `myapp-svc-abc123`) |
No description provided.