feat(packages): support configurable git URL in package generation#906
feat(packages): support configurable git URL in package generation#906ti-chi-bot[bot] merged 7 commits intomainfrom
Conversation
templates The commit adds support for configurable Git URLs in package templates by: 1. Adding `.Git.url` to the list of available template variables in README.md 2. Updating all package components to use the new `.Git.url` template variable with default values 3. Modifying build scripts to support passing a custom Git URL 4. Adding default Git URLs for image source labels in build scripts This change allows for more flexible configuration of Git repository URLs in the package generation process.
add default git url to ci script and update package scripts to always set git url
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces support for configurable Git URLs in package generation by using context variables (.Git.url) in scripts and templates. It ensures that release workflows can override source repositories dynamically. The approach involves updating scripts and templates to accept and process the new .Git.url variable, with sensible defaults for backward compatibility. Overall, the implementation is consistent and adheres to the existing structure, though there are areas for improvement in error handling and testing coverage.
Critical Issues
- Missing Validation for
DEFAULT_GIT_URLin.github/scripts/ci.sh- File:
.github/scripts/ci.sh, various lines (e.g., line 8, 38, 52, etc.) - Issue: The
DEFAULT_GIT_URLvariable is hardcoded but lacks validation to ensure it is a valid URL. If the URL is malformed, it could lead to runtime failures when invoking scripts. - Suggestion: Add a validation step for
DEFAULT_GIT_URLat the start of the script:if ! [[ "$DEFAULT_GIT_URL" =~ ^(git@|https://).* ]]; then echo "Invalid DEFAULT_GIT_URL: $DEFAULT_GIT_URL" >&2 exit 1 fi
- File:
Code Improvements
- Error Handling for Missing
.Git.urlin Templates- File:
packages/packages.yaml.tmpl, various lines (e.g., lines 29, 59, 266, etc.) - Issue: While the templates use
defaultfor.Git.url, there is no explicit error handling for cases wheredefaultis overridden but.Git.urlis still invalid or empty. - Suggestion: Introduce validation logic within the scripts or templates to ensure that
.Git.urlis always a valid URL and never empty. For instance, a fallback mechanism could be added to log warnings if.Git.urlis invalid.
- File:
Best Practices
-
Documentation for
.Git.urlContext- File:
packages/README.md, line 40 - Issue: The addition of
.Git.urlto the list of context variables is helpful but lacks a detailed explanation of its purpose and usage. - Suggestion: Expand the documentation to clarify:
- When
.Git.urlshould be overridden. - Examples of valid
.Git.urlvalues (e.g., HTTPS vs SSH URLs). - How
.Git.urlinteracts with other variables like.Git.ref.
- When
- File:
-
Testing Coverage for SSH URL Handling
- File: Various scripts (e.g.,
.github/scripts/ci.sh,packages/scripts/gen-package-artifacts-with-config.sh) - Issue: The PR description mentions validation for SSH-style Git URLs (e.g.,
git@github.com:...), but there is no evidence of automated tests ensuring this functionality works. - Suggestion: Add automated tests that cover:
- SSH URL parsing and injection into templates.
- Cases where
.Git.urlis overridden with invalid or empty values. - Ensure backward compatibility by testing scenarios where
.Git.urlremains unset.
- File: Various scripts (e.g.,
-
Default Value in Metadata Labels
- File:
packages/scripts/build-package-artifacts.sh.tmpl, line 253;packages/scripts/build-package-images.sh.tmpl, line 161 - Issue: The default value for
.Git.urlin metadata labels (https://github.com/unknown/unknown.git) is generic and could lead to confusion. - Suggestion: Replace
"https://github.com/unknown/unknown.git"with"https://example.com/unknown.git"or another clearer placeholder to indicate intentional fallback.
- File:
Suggested Code Refinements
Improved Error Handling for .Git.url Validation
Add validation in scripts to ensure .Git.url is always a valid URL:
if [[ -z "$git_url" || ! "$git_url" =~ ^(git@|https://).* ]]; then
echo "Error: Invalid Git URL provided: $git_url" >&2
exit 1
fiTest Case Sample for SSH URL Handling
Introduce a test script to validate .Git.url overrides:
#!/bin/bash
DEFAULT_GIT_URL="git@github.com:pingcap/tidb.git"
function test_git_url_handling() {
local test_url=$1
local expected=$2
result=$(yq ".Git.url = \"$test_url\"" release-context.yaml)
if [[ "$result" != "$expected" ]]; then
echo "Test failed: Expected $expected but got $result" >&2
exit 1
else
echo "Test passed for $test_url"
fi
}
test_git_url_handling "git@github.com:pingcap/tidb.git" "git@github.com:pingcap/tidb.git"
test_git_url_handling "https://github.com/pingcap/tidb.git" "https://github.com/pingcap/tidb.git"
test_git_url_handling "" "https://github.com/unknown/unknown.git"Summary of Improvements
This PR is well-structured overall but could benefit from better error handling, documentation clarity, and testing coverage for edge cases. Addressing these points will improve reliability and maintainability of the code.
Summary of ChangesHello, 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 significantly enhances the flexibility of the package generation system by introducing configurable Git URLs. This change allows for greater control over source repositories during release processes, enabling workflows to easily override default Git URLs for individual components. The update involves modifications to core package templates, generator scripts, and CI configurations to ensure the new Highlights
Changelog
Activity
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 support for a configurable git URL in package generation. However, it introduces several injection vulnerabilities by using untrusted input (like git URLs and registry paths) directly in shell commands and yq expressions without proper sanitization or escaping. Specifically, changes in build-package-images.sh.tmpl and build-package-artifacts.sh.tmpl can lead to command and expression injection in generated CI scripts, potentially allowing arbitrary code execution on CI runners. It is recommended to use environment variables to pass these values safely. Beyond the security concerns, there are critical issues in the CI script (.github/scripts/ci.sh) where generator scripts are called with incorrect arguments, leading to CI failures due to empty strings overriding default file paths. Additionally, a potential design flaw exists with the global .Git.url override, which might result in incorrect repository URLs for components not residing in the main pingcap/tidb repository.
| kaniko_global_options="$kaniko_global_options --label org.opencontainers.image.title={{ .component }}" | ||
| kaniko_global_options="$kaniko_global_options --label org.opencontainers.image.licenses={{ .license | default "Apache-2.0" }}" | ||
| kaniko_global_options="$kaniko_global_options --label org.opencontainers.image.source={{ .git.url }}" | ||
| kaniko_global_options="$kaniko_global_options --label org.opencontainers.image.source={{ .git.url | default "https://github.com/unknown/unknown.git" }}" |
There was a problem hiding this comment.
The template renders .git.url directly into a shell command string (kaniko_global_options). If .git.url contains shell metacharacters like $(...) or backticks, it will lead to command injection when the generated build-package-images.sh script is executed. An attacker controlling the git URL (e.g., via a malicious branch name or PR parameter) could execute arbitrary commands on the CI runner.
To remediate this, wrap the value in single quotes and ensure it doesn't contain single quotes, or better, pass it as an environment variable.
| for ac in $architectures; do | ||
| echo -en "[🚢] $cm $os $ac $version $profile:\t" | ||
| img=$($script "$cm" "$os" "$ac" "$version" "$profile") | ||
| img=$($script "$cm" "$os" "$ac" "$version" "$profile" "" "" "$DEFAULT_GIT_URL") |
There was a problem hiding this comment.
The script get-package-builder-with-config.sh is being called with 8 arguments, but it only expects up to 7. The extra arguments "" "" "$DEFAULT_GIT_URL" seem to be a copy-paste error from other test functions. The script get-package-builder-with-config.sh does not accept a git URL as an argument; it was modified to statically set an empty URL. Furthermore, passing "" as the 6th and 7th arguments will override the default values for template_file and out_file with empty strings, which will likely cause the script to fail. This call should probably be reverted to its original form with 5 arguments to use the default values for template_file and out_file.
| img=$($script "$cm" "$os" "$ac" "$version" "$profile" "" "" "$DEFAULT_GIT_URL") | |
| img=$($script "$cm" "$os" "$ac" "$version" "$profile") |
This change removes the hardcoded default Git URL from the CI script, allowing each component to use its own default repository URL instead. This makes the script more flexible and component-specific.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces support for configurable Git URLs in package generation workflows, allowing per-component repository overrides through .Git.url in the context variables. The implementation updates template files, scripts, and CI workflows to accommodate this change, while maintaining default values for backward compatibility. The overall quality is solid, with proper integration across affected scripts and templating. However, there are some minor design and maintainability concerns that could be improved.
Critical Issues
None identified. The changes appear functional, with no immediate bugs, security vulnerabilities, or broken functionality.
Code Improvements
-
Hardcoded Registry Defaults:
- File:
packages/scripts/gen-package-artifacts-with-config.sh(line 11) andpackages/scripts/gen-package-images-with-config.sh(line 11). - Issue: The hardcoded default registry (
us-docker.pkg.dev/pingcap-testing-account/hub) could make the scripts less portable or harder to adapt to different environments without modification. - Suggestion: Use a configuration file or environment variable to define the default registry, falling back to the hardcoded value if not set. Example:
local registry="${10:-${DEFAULT_REGISTRY:-us-docker.pkg.dev/pingcap-testing-account/hub}}"
- File:
-
Redundant Context Updates:
- File: Multiple scripts (e.g.,
gen-package-artifacts-with-config.sh,gen-package-images-with-config.sh,get-package-builder-with-config.sh). - Issue: The repeated use of
yq -icommands to update context variables (.Git.ref,.Git.sha,.Git.url, etc.) could be consolidated for better readability and maintainability. - Suggestion: Group the updates into a single block or function to reduce duplication. Example:
function update_context() { local ref="$1" local sha="$2" local url="$3" yq -i ".Git.ref = \"$ref\"" release-context.yaml yq -i ".Git.sha = \"$sha\"" release-context.yaml yq -i ".Git.url = \"$url\"" release-context.yaml }
- File: Multiple scripts (e.g.,
Best Practices
-
Missing Documentation for New
.Git.urlContext Variable:- File:
packages/README.md. - Issue: While
.Git.urlis briefly added to the list of context variables, there is no explanation regarding its usage, format, or the impact of its omission. - Suggestion: Expand the README to describe
.Git.url, including acceptable formats (e.g., HTTPS, SSH), default behavior, and examples of usage.
- File:
-
Testing Coverage:
- Issue: The PR description mentions validation steps such as
bash -nand smoke tests, but no details about automated testing for edge cases like invalid.Git.urlvalues are provided. - Suggestion: Add tests to verify behavior when
.Git.urlis invalid, missing, or overridden with non-standard formats. For example:- Missing
.Git.urlshould fall back to component-specific defaults. - Invalid
.Git.url(e.g., malformed SSH/Git URL) should raise a clear error.
- Missing
- Issue: The PR description mentions validation steps such as
-
Default Git URL in Templates:
- File:
packages/packages.yaml.tmpl(multiple lines). - Issue: The default Git URL fallback in the templates (
default "https://github.com/{component}.git") is consistent but may lead to confusion if the repository structure changes or additional repositories are introduced. - Suggestion: Consider centralizing the default Git URL logic in a configuration file or environment variable to ensure maintainability.
- File:
Example for centralized configuration:
# defaults.yaml
default_git_url: "https://github.com/{component}.git"Then reference defaults.yaml in templates.
Summary of Suggested Changes
- Replace hardcoded registry defaults with environment variables or configuration files.
- Refactor redundant
yq -iupdates into a reusable function. - Enhance documentation for
.Git.urlinpackages/README.md. - Add tests for edge cases (e.g., missing or invalid
.Git.url). - Centralize the default Git URL fallback logic for easier maintenance.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces support for configurable Git URLs in package generation workflows by leveraging .Git.url with per-component defaults. The implementation modifies template files, helper scripts, and CI scripts to ensure the .Git.url variable is passed and processed consistently, with support for SSH-style URLs. The approach is generally well-structured and maintains backward compatibility through default values. However, there are areas where error handling, documentation, and testing coverage can be improved.
Critical Issues
- Potential runtime error when
git_urlis unset or malformed (multiple files):- Files:
packages/scripts/build-package-artifacts.sh.tmpl,packages/scripts/build-package-images.sh.tmpl - Issue: If
GIT_URLis unset or malformed, the scripts default to"https://github.com/unknown/unknown.git". This could lead to invalid metadata or failed builds. - Suggested Solution: Validate
GIT_URLearly in the script and exit with an error if it is empty or invalid.if [[ -z "$GIT_URL" || ! "$GIT_URL" =~ ^(https://|git@) ]]; then echo "Error: Invalid or missing GIT_URL" exit 1 fi
- Files:
Code Improvements
-
Repeated use of default values across template files:
- Files:
packages/packages.yaml.tmpl - Issue: The default Git URL is repeatedly specified for every component using
{{ .Git.url | default "..." }}. This introduces redundancy and increases maintenance overhead. - Suggested Solution: Define a global default value for
.Git.urland reference it wherever needed.default_git_url: "https://github.com/unknown/unknown.git" components: advanced-statefulset: git: url: '{{ .Git.url | default .default_git_url }}'
- Files:
-
Hard-coded registry values in scripts:
- Files:
packages/scripts/gen-package-artifacts-with-config.sh,packages/scripts/gen-package-images-with-config.sh - Issue: The registry defaults to
us-docker.pkg.dev/pingcap-testing-account/hub, which may not be appropriate for all environments. - Suggested Solution: Pass the registry as a parameter or use an environment variable to make it configurable.
- Files:
-
Error handling for
yqcommands:- Files:
packages/scripts/gen-package-artifacts-with-config.sh,packages/scripts/gen-package-images-with-config.sh,packages/scripts/get-package-builder-with-config.sh - Issue: The
yqcommands assume successful execution but do not check for errors. Ifyqfails, the script could proceed with incorrect or incomplete context. - Suggested Solution: Add error handling after each
yqinvocation.yq -i ".Git.url = \"$git_url\"" release-context.yaml || { echo "Error: Failed to set Git URL in release-context.yaml" exit 1 }
- Files:
Best Practices
-
Missing comments for new parameters:
- Files:
.github/scripts/ci.sh,packages/scripts/gen-package-artifacts-with-config.sh,packages/scripts/gen-package-images-with-config.sh,packages/scripts/get-package-builder-with-config.sh - Issue: The new
DEFAULT_GIT_URLandgit_urlparameters lack explanations in comments, making it harder to understand their purpose. - Suggested Solution: Add comments describing the role of these variables.
# DEFAULT_GIT_URL: Fallback Git repository URL for components without explicit definitions. readonly DEFAULT_GIT_URL=""
- Files:
-
Testing coverage gaps:
- Files: Scripts across
.github/scripts/andpackages/scripts/ - Issue: While smoke tests were run, there is no mention of validation for edge cases, such as malformed Git URLs, missing registry settings, or failures in template rendering.
- Suggested Solution: Add unit tests for the scripts to cover scenarios like:
- Missing
git_url. - Invalid or unsupported URL formats.
- Template rendering errors.
- Missing
- Files: Scripts across
-
Documentation updates:
- Files:
packages/README.md - Issue: The README mentions
.Git.urlas a new context variable but doesn't explain its usage or the implications of overriding defaults. - Suggested Solution: Expand the documentation with examples of
.Git.urlusage and how it interacts with the templates.### Configurable Git URLs The `.Git.url` context variable allows specifying custom Git repository URLs for components. If unset, a default URL is used. Example usage: ```yaml components: tidb: git: url: '{{ .Git.url | default "https://github.com/pingcap/tidb.git" }}'
- Files:
-
Style guideline deviations:
- Files: Modified Bash scripts (
ci.sh, others) - Issue: Inline string concatenation in Bash (e.g.,
"$kaniko_global_options ... --label org.opencontainers.image.source=${source_url}") lacks readability. - Suggested Solution: Use multi-line concatenation for clarity.
kaniko_global_options+=" --label org.opencontainers.image.source=${source_url}" kaniko_global_options+=" --label org.opencontainers.image.ref.name={{ .git.ref }}"
- Files: Modified Bash scripts (
Final Notes
The PR is functional and achieves its goal of adding configurable Git URLs, but addressing the critical issues and improvements above will enhance its robustness, maintainability, and usability.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces support for configurable Git URLs in package generation by reading .Git.url from context variables, allowing per-component defaults and overrides. It updates package templates, scripts, and CI configurations to handle these URLs, including SSH-style formats. The approach uses template changes with fallback defaults and script updates to dynamically inject Git URLs into metadata. The changes are generally well-structured, but there are areas for improvement in error handling, input validation, and adherence to best practices.
Critical Issues
-
Missing Validation for
DEFAULT_GIT_URL- File:
.github/scripts/ci.sh, multiple lines - Issue: The
DEFAULT_GIT_URLis initialized as an empty string, and there is no validation to ensure it is set or correct before use. This could lead to unexpected behavior if the variable remains unset during execution. - Suggestion: Add validation to check if
DEFAULT_GIT_URLis set and matches a valid URL format early in the script. Example:if [[ -z "${DEFAULT_GIT_URL}" ]]; then echo "Error: DEFAULT_GIT_URL is not set." exit 1 fi if ! [[ "${DEFAULT_GIT_URL}" =~ ^(https:\/\/|git@).* ]]; then echo "Error: DEFAULT_GIT_URL is not a valid URL." exit 1 fi
- File:
-
Potential Insecure Handling of
source_url- File:
packages/scripts/build-package-artifacts.sh.tmpl,packages/scripts/build-package-images.sh.tmpl, multiple lines - Issue:
source_urlis dynamically derived from theGIT_URLenvironment variable or context, but there is no validation to ensure it is a trusted or valid URL. This creates a potential security risk, especially if the variable is externally influenced. - Suggestion: Validate
source_urlbefore use, ensuring it adheres to expected patterns (e.g., HTTPS or SSH). Example:if ! [[ "${source_url}" =~ ^(https:\/\/|git@).* ]]; then echo "Error: source_url is invalid or insecure." exit 1 fi
- File:
Code Improvements
-
Refactor Redundant Context Updates
- Files:
gen-package-artifacts-with-config.sh,gen-package-images-with-config.sh,get-package-builder-with-config.sh - Issue: Context updates using
yqare repeated multiple times for different variables. This redundancy makes the scripts harder to maintain. - Suggestion: Use a loop to update the context for all variables dynamically. Example:
declare -A context_vars=( ["Release.os"]="$os" ["Release.arch"]="$arch" ["Release.version"]="$version" ["Release.profile"]="$profile" ["Release.registry"]="$registry" ["Git.ref"]="$git_ref" ["Git.sha"]="$git_sha" ["Git.url"]="$git_url" ) for key in "${!context_vars[@]}"; do value="${context_vars[$key]}" yq -i ".${key} = strenv(value)" release-context.yaml done
- Files:
-
Improve Default Handling in Templates
- File:
packages/packages.yaml.tmpl, multiple lines - Issue: Fallback defaults for
.Git.urlare hardcoded in multiple places, increasing the risk of inconsistency if defaults need to be updated. - Suggestion: Define a single default value as a variable at the top of the template, and reference it where needed. Example:
{{- $defaultGitUrl := "https://github.com/unknown/unknown.git" -}} url: '{{ .Git.url | default $defaultGitUrl }}'
- File:
Best Practices
-
Testing Coverage for Edge Cases
- Files: All updated scripts
- Issue: The validation section mentions smoke tests, but there is no evidence of testing edge cases like invalid Git URLs, unset environment variables, or missing context keys.
- Suggestion: Add tests to cover these scenarios, such as passing invalid or missing
DEFAULT_GIT_URLvalues, and ensure scripts handle them gracefully.
-
Documentation Updates for CI Scripts
- File:
.github/scripts/ci.sh - Issue: The script now relies on
DEFAULT_GIT_URL, but this new dependency is not documented. - Suggestion: Update comments and README documentation to explain the role of
DEFAULT_GIT_URLand its required format (e.g., HTTPS or SSH).
- File:
-
Consistent Naming for Variables
- Files: Multiple scripts (e.g.,
ci.sh,gen-package-artifacts-with-config.sh) - Issue: Variable names like
DEFAULT_GIT_URL,GIT_URL, andsource_urlare inconsistent in capitalization conventions. - Suggestion: Use a consistent naming convention, such as
DEFAULT_GIT_URLordefault_git_url, across all scripts.
- Files: Multiple scripts (e.g.,
Summary of Suggested Changes
- Add validation for critical variables (
DEFAULT_GIT_URL,source_url) to prevent runtime errors and security risks. - Refactor repetitive context updates in scripts using loops or functions.
- Consolidate fallback defaults in templates to avoid duplication.
- Expand test coverage for edge cases and update documentation to reflect new dependencies and functionality.
- Standardize variable naming conventions for clarity and consistency.
These changes will improve maintainability, security, and robustness of the updated package generation process.
|
Addressed the review comments in latest commits:
Validation done:
|
This reverts commit 54b1221.
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This PR introduces support for configuring Git URLs in package generation workflows. It modifies packages.yaml.tmpl to allow git.url to be dynamically overridden via .Git.url context, with default values provided for each component. The bash scripts involved in CI and package generation have been updated to handle this new configuration. Overall, the changes are structured logically with clear intent, though some areas require refinement for improved maintainability, robustness, and adherence to best practices.
Critical Issues
-
Missing validation for
DEFAULT_GIT_URLin.github/scripts/ci.sh- File:
.github/scripts/ci.sh, Lines: multiple - Issue: The
DEFAULT_GIT_URLenvironment variable is used without any checks for validity. If it is empty or malformed, it could lead to unexpected behavior in downstream scripts. - Suggested Solution: Add validation for
DEFAULT_GIT_URLto ensure it is non-empty and conforms to a valid URL format. For example:if [[ -z "$DEFAULT_GIT_URL" || ! "$DEFAULT_GIT_URL" =~ ^(https?|git@) ]]; then echo "Error: DEFAULT_GIT_URL is invalid or not set." exit 1 fi
- File:
-
Potential security issue with dynamic Git URL injection
- Files:
packages/scripts/build-package-artifacts.sh.tmpl,packages/scripts/build-package-images.sh.tmpl - Issue: The usage of
${GIT_URL}allows external inputs for URL configuration. Without sanitization, this could introduce security risks such as command injection or misuse. - Suggested Solution: Sanitize the
GIT_URLinput and ensure it only allows trusted formats. Consider validating URLs before injecting them into scripts.
- Files:
Code Improvements
-
Refactor repetitive code in
packages.yaml.tmpl- File:
packages/packages.yaml.tmpl, Lines: multiple - Issue: The
git.urlfield for all components has been updated with identical logic usingdefault. This introduces redundancy and makes later updates error-prone. - Suggested Solution: Extract the common
git.urllogic into a reusable helper template and reference it in each component. For example:{{ define "default_git_url" }} '{{ .Git.url | default "https://github.com/pingcap/{{ .component }}.git" }}' {{ end }} components: advanced-statefulset: git: url: {{ template "default_git_url" . }}
- File:
-
Improve error handling in script functions
- Files:
*.shscripts, Lines: multiple - Issue: Functions like
prepare_artifact_configandbuild_and_push_imageslack robust error handling. Failures in commands likeyqorgomplatecould go unnoticed, leading to incomplete or corrupt outputs. - Suggested Solution: Add error handling for critical commands. Example:
gomplate --context .=release-context.yaml -f "$template_file" --out release-packages.yaml || { echo "Error: Failed to generate release packages." exit 1 }
- Files:
Best Practices
-
Improve documentation for new
git.urlcontext variable- File:
packages/README.md, Line: 40 - Issue: The addition of
.Git.urlis briefly mentioned but lacks explanation about its purpose, default behavior, and usage scenarios. - Suggested Solution: Expand the documentation to include these details. Example:
`.Git.url`: The repository URL for the component. Defaults to the official repository URL unless overridden in the release context. Supports HTTPS and SSH formats.
- File:
-
Add unit tests for Git URL override functionality
- Files: No testing changes in the diff
- Issue: The PR modifies critical logic for Git URL handling but lacks any automated tests to validate this new functionality.
- Suggested Solution: Add tests to ensure dynamic Git URLs are correctly processed and fallback to defaults when not provided. For example, tests could assert correct URL generation in template parsing and script execution.
-
Style deviations in bash scripts
- Files:
.github/scripts/ci.sh, Lines: multiple - Issue: Some lines have inconsistent indentation or lack quotes around variable usage (e.g.,
$script "$cm"). - Suggested Solution: Follow standard bash style guidelines, such as quoting all variables and aligning indentation properly:
img=$("$script" "$cm" "$os" "$ac" "$version" "$profile" "" "" "$DEFAULT_GIT_URL")
- Files:
-
Avoid hardcoding registry values in scripts
- Files:
packages/scripts/gen-package-artifacts-with-config.sh,packages/scripts/gen-package-images-with-config.sh - Issue: Default registry values (
us-docker.pkg.dev/pingcap-testing-account/hub) are hardcoded, which reduces flexibility. - Suggested Solution: Define the registry values in a configuration file or use environment variables to make them configurable.
- Files:
Conclusion
While the PR effectively implements configurable Git URLs, it introduces several risks due to insufficient validation and error handling. Addressing these issues and improving code maintainability will enhance the robustness and security of the changes. Focus on validation, refactoring repetitive logic, and adding appropriate testing before merging.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
1 participant
Summary
packages/packages.yaml.tmplreadgit.urlfrom.Git.urlwith per-component defaults so release workflows can override source repos.Git.urlcontext, including support for SSH-style URLs in generated metadata labels.Git.urlto docs/context variablesValidation
bash -nfor updated scripts:.github/scripts/ci.shand package generator/build helpers./.github/scripts/ci.shpast the original template rendering failuregit@github.com:pingcap/tidb.gitwith builder/image/artifact script generation