Conversation
Signed-off-by: liubo02 <liubo02@pingcap.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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 enhances the project's build system by introducing a dedicated configuration for the 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
|
![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 adds a new build router configuration to the packages.yaml.tmpl to build the tidb-monitor-reloader Docker image for tags matching the pattern reloader/vX.Y.Z. The approach is to insert a conditional router entry that triggers image building for specific git refs, targeting Linux OS on amd64 and arm64 architectures. The change is straightforward and fits well into the existing build router structure.
Code Improvements:
-
File:
packages/packages.yaml.tmpl(lines ~295-308)- Issue: The regex pattern used in
ifcondition is^reloader/v[0-9]+\.[0-9]+\.[0-9]+$.- This regex is mostly correct but could be more robust by anchoring the end properly and escaping the dot correctly (which is done here). However, it does not cover possible pre-release or build metadata in semver tags. If your releases might include those, consider expanding the regex or using semver parsing instead, for consistency.
- Suggestion: If you want to be consistent and robust, consider reusing semver parsing like in other conditionals. Example:
or simply document that only strict
if: {{ and (regexp.Match "^reloader/v[0-9]+\\.[0-9]+\\.[0-9]+$" .Git.ref) (semver.Parse(strings.TrimPrefix(.Git.ref, "reloader/v"))) }}
vX.Y.Ztags are handled. - Reason: This avoids accidental triggering on malformed tags and aligns with other semver conditionals.
- Issue: The regex pattern used in
-
File:
packages/packages.yaml.tmpl- Issue: The
contextis set toreload, anddockerfiletoDockerfile_buildxwithout path clarification.- Make sure the
reloaddirectory exists at the expected path relative to the build context root, and thatDockerfile_buildxis inside it or the path is correct.
- Make sure the
- Suggestion: Add a comment or ensure that this path is consistent and maintained, to prevent build errors.
- Issue: The
Best Practices:
-
File:
packages/packages.yaml.tmpl(around line 295)- Issue: No PR description provided.
- Suggestion: Add a brief PR description explaining the purpose of adding this router (build the new
tidb-monitor-reloaderimage for specific tags) to improve maintainability and onboarding of reviewers.
-
Testing Coverage:
- Verify that the build pipeline is tested or exercised with a matching git ref, to ensure the new router triggers correctly and the image is built as expected.
-
Documentation:
- Consider documenting this new image build in the repo’s build or release documentation, so users and maintainers know about this new image and how it is triggered.
No critical issues found. The change is small and focused but would benefit from a bit more documentation and possibly slight regex enhancement for robustness.
![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 adds a new build definition for the tidb-monitor-reloader image. The change is mostly correct, but there's a potential issue with image tagging. The new artifact inherits tags from the parent monitoring component, which can lead to incorrect version tags. I've suggested a fix to ensure the image is tagged correctly based on its own versioning scheme.
| artifactory: | ||
| repo: "{{ .Release.registry }}/pingcap/tidb-monitor-reloader/image" |
There was a problem hiding this comment.
This artifact is defined within the monitoring component, so it inherits the component-level artifactory.tags. This includes {{ .Release.version }}, which is likely the TiDB release version and not the version of tidb-monitor-reloader. This can lead to confusing or incorrect image tags.
To ensure correct versioning for the tidb-monitor-reloader image, it's better to override the tags at the artifact level, deriving the version from the .Git.ref.
artifactory:
repo: "{{ .Release.registry }}/pingcap/tidb-monitor-reloader/image"
tags:
{{- $version := strings.TrimPrefix "reloader/" .Git.ref }}
{{- if .Git.sha }}
- {{ $version }}-{{ strings.Trunc 7 .Git.sha }}
{{- end }}
- {{ $version }}
packages/packages.yaml.tmpl
Outdated
| context: reload | ||
| dockerfile: Dockerfile_buildx | ||
| - description: For range [v8.4.0, ) | ||
| if: {{ semver.CheckConstraint ">= 8.4.0-0" .Release.version }} |
There was a problem hiding this comment.
All existing monitoring routes are added with "exclude reloader prefix".
··· if: {{ and (not (strings.HasPrefix "reloader/" .Git.ref)) (semver.CheckConstraint ">= 8.4.0-0" .Release.version) }} ···
The same logic applies to other following semver judgment.
packages/packages.yaml.tmpl
Outdated
| artifactory: | ||
| repo: "{{ .Release.registry }}/pingcap/tidb-monitor-reloader/image" | ||
| context: reload | ||
| dockerfile: Dockerfile_buildx |
There was a problem hiding this comment.
ref: https://github.com/pingcap/monitoring/blob/master/reload/Dockerfile_buildx
reload/Dockerfile_buildx contains COPY go.mod go.sum ./ and COPY reload ./reload, which rely on the repository root directory as the context.
It is recommended to change to context: . + dockerfile: reload/Dockerfile_buildx.
packages/packages.yaml.tmpl
Outdated
| artifactory: | ||
| repo: "{{ .Release.registry }}/pingcap/tidb-monitor-reloader/image" | ||
| context: reload | ||
| dockerfile: Dockerfile_buildx |
There was a problem hiding this comment.
Need to add a delivery rule under image_copy_rules in packages/delivery.yaml; otherwise, the synchronization will not be performed.
Signed-off-by: liubo02 <liubo02@pingcap.com>
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 adds support for building and delivering the tidb-monitor-reloader image. It introduces the image definition in delivery.yaml with relevant version tagging rules and updates packages.yaml.tmpl to add a build router triggered by Git refs matching the reloader/vX.Y.Z pattern. The existing routing rules are also adjusted to exclude those refs. Overall, the changes are straightforward and appear consistent with the project's delivery and build system patterns.
Code Improvements
-
File:
packages/packages.yaml.tmpl(lines ~290-310)- Issue: The regex pattern in the router's
ifcondition uses double backslashes to escape dots in Go templating, which can be error-prone and hard to read:if: {{ regexp.Match "^reloader/v[0-9]+\\.[0-9]+\\.[0-9]+$" .Git.ref }}
- Why: Escaping is correct but can be fragile if modified or copy-pasted incorrectly.
- Suggestion: Define the regex as a variable above or comment to clarify the pattern. Alternatively, consider using a semantic version parser if available to improve clarity and robustness.
- Issue: The regex pattern in the router's
-
File:
packages/packages.yaml.tmpl(lines ~295-305)- Issue: The new router for
tidb-monitor-reloaderusescontext: .anddockerfile: reload/Dockerfile_buildx. - Why: The PR does not include the Dockerfile or context directory. If missing, this will break builds.
- Suggestion: Confirm and include the
reload/Dockerfile_buildxand context directory in the repository. Document this dependency in the PR description.
- Issue: The new router for
-
File:
packages/packages.yaml.tmpl(lines ~300-400)- Issue: Multiple routing conditions are updated to exclude the new
reloader/prefixed refs by adding(not (strings.HasPrefix "reloader/" .Git.ref)). - Why: This prevents conflicts but duplicates exclusion logic in many places.
- Suggestion: Consider centralizing this exclusion condition or adding a helper function in the templating to reduce duplication and risk of missing future updates.
- Issue: Multiple routing conditions are updated to exclude the new
Best Practices
-
File:
packages/delivery.yaml(lines ~384-395)- Issue: The image description string
"delivery the version images."is grammatically unclear. - Why: Clear descriptions help maintainers understand the purpose of image entries.
- Suggestion: Change to a clearer description, e.g.:
- description: Deliver tidb-monitor-reloader versioned images.
- Issue: The image description string
-
Documentation:
- The PR description is minimal.
- Suggestion: Expand the PR description to explain:
- What the
tidb-monitor-reloaderimage is and its purpose. - How the new router triggers builds based on Git refs.
- Any prerequisite files or directories (Dockerfile, context).
- Expected impact on existing build and delivery processes.
- What the
-
Testing Coverage:
- No mention of testing or validation steps for the new image build.
- Suggestion: Add or mention tests or CI jobs that verify the
tidb-monitor-reloaderimage builds and publishes correctly.
Summary of Actionable Suggestions
- Add or verify presence of `reload/Dockerfile_buildx` and context directory.
- Improve regex pattern readability or add comments explaining it.
- Centralize the exclusion of `reloader/` refs in routing conditions to reduce repetition.
- Clarify image description in `delivery.yaml`.
- Expand PR description to include purpose, build triggers, and impact.
- Add testing or CI validation for the new image build.Addressing these points will improve maintainability, clarity, and robustness of the newly introduced image build process.
There was a problem hiding this comment.
Pull request overview
Adds build and delivery configuration for a new tidb-monitor-reloader container image, triggered by reloader/vX.Y.Z git tags in the monitoring component pipeline.
Changes:
- Add a new build router in
packages.yaml.tmplto build/publishtidb-monitor-reloaderforreloader/vX.Y.Zrefs, and excludereloader/*refs from existing monitoring build routers. - Add image copy/delivery rules for
tidb-monitor-reloaderversion tags to Docker Hub (and mirrored registries) indelivery.yaml.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/packages.yaml.tmpl | Adds a dedicated router to build/publish tidb-monitor-reloader images for reloader/vX.Y.Z tags and prevents other monitoring routers from matching those refs. |
| packages/delivery.yaml | Adds copy rules to deliver tidb-monitor-reloader images for vX.Y.Z tags to downstream registries. |
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
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 adds support for building and delivering a new tidb-monitor-reloader image, including updating delivery rules and package build templates. The approach introduces a new image definition with versioned tags regex, and a build router triggered by Git references prefixed with reloader/. The overall changes are straightforward and consistent with existing patterns. The patch is mostly clean and well integrated.
Code Improvements
-
Improve Regex Consistency and Robustness
- File:
packages/packages.yaml.tmpllines ~291-304 - Issue: The regex for matching the Git ref
"^reloader/v[0-9]+\\.[0-9]+\\.[0-9]+$"is manually escaped with double backslashes. Since this is a Go template usingregexp.Match, verify if double escaping is necessary or if a raw string literal can be used to improve readability and avoid errors. - Suggestion: Use raw string notation (
backticks) or single backslash if supported by the templating engine, e.g.:This reduces escape complexity and potential mistakes.if: {{ regexp.Match `^reloader/v[0-9]+\.[0-9]+\.[0-9]+$` .Git.ref }}
- File:
-
Clarify "if" Conditions for Other Versions
- File:
packages/packages.yaml.tmpl(multiple lines, e.g., 305-420) - Issue: The introduction of
and (not (strings.HasPrefix "reloader/" .Git.ref))in all other version checks is logical but slightly repetitive and could risk missing future version ranges. - Suggestion: Consider refactoring the conditional logic to isolate the
reloader/handling earlier or as a separate conditional block to improve maintainability and reduce duplication.
- File:
Best Practices
-
Add Comments Explaining the New Image and Build Router
- Files:
packages/delivery.yamllines ~384-394,packages/packages.yaml.tmpllines ~290-310 - Issue: The new image definition and build router lack descriptive comments explaining the purpose of the
tidb-monitor-reloader, its usage, and why the routing is based onreloader/prefixes. - Suggestion: Add brief comments to clarify the intent for future maintainers, e.g.:
# tidb-monitor-reloader image definition for versioned releases. # This image handles ... (brief purpose).
- Files:
-
Test Coverage for New Image Build and Delivery
- Files: Not shown in diff but implied
- Issue: No mention of adding or updating tests for the new image build or delivery logic.
- Suggestion: Ensure CI includes tests that verify the new
reloader/vX.Y.Ztag triggers the correct build and delivery flow.
Minor Improvements
- Typo Fix in Delivery Description
- File:
packages/delivery.yamlline 12 - Issue: Changed description from
"delivery the trunk branch community images."to"deliver trunk branch community images."which is correct. Good fix.
- File:
No critical issues identified. The PR introduces a meaningful feature with proper integration into existing build and delivery infrastructure but would benefit from enhanced documentation and minor refactoring for maintainability.
Uh oh!
There was an error while loading. Please reload this page.