fix(packages): add br-federation-manager image packaging and delivery for v1.x #876
Conversation
Summary of ChangesHello @wuhuizuo, 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 packaging and delivery pipeline by incorporating the Highlights
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 packaging and delivery support for the br-federation-manager container image in the v1.x release line. The approach involves updating the delivery.yaml to define image copy rules and extend packages.yaml.tmpl to include build instructions for this new image, aligning with existing patterns. The changes are straightforward and well-structured, correctly scoped, and seem consistent with the current packaging setup.
Code Improvements
-
packages/delivery.yaml(lines 156-165)- The
tags_regexpattern is designed to exclude pre-release tags with a negative lookahead:-(?!pre(\.|$)). This is mostly fine, but the regex might miss some edge cases if any pre-release tags use different separators or formats. - Suggestion: Consider slightly broadening or documenting the regex to clarify what pre-release tags are excluded. For example, if other pre-release labels exist (e.g.,
-alpha,-beta), they will be included, which may or may not be intentional.
- The
-
packages/packages.yaml.tmpl(lines 1654-1672, 1698-1716)- The
build_argsonly specifyTARGETARCH={{ .Release.arch }}. Confirm whether additional build args (likeVERSIONorBUILD_DATE) are needed for reproducible or traceable images, similar to other images in the repo. - The
contextanddockerfilepaths are relative (images/br-federation-manager,Dockerfile). Verify these paths exist and that the Dockerfile supports multi-arch builds keyed byTARGETARCH. - The image name and repo string are consistent, but consider consistency in naming in documentation and tooling (e.g., is it always
pingcap/tidb-operator/images/br-federation-manageror should it bepingcap/br-federation-managerelsewhere?).
- The
Best Practices
-
Documentation
- No comments or descriptions were added describing the purpose of
br-federation-managerin the YAML files. Adding a brief comment ordescriptionfield would help future maintainers understand the new image’s role in the packaging and delivery pipeline.
- No comments or descriptions were added describing the purpose of
-
Testing Coverage
- No indication of testing or CI modifications to build and push the new image. Ensure the relevant CI pipelines are updated to build and deliver this image as intended, else the addition remains unused.
- Consider adding or updating automated tests that verify the image copy rules and build steps for
br-federation-manager.
-
Naming Conventions & Consistency
- The new image is consistently named
br-federation-manager. Ensure this matches all related tooling and documentation to avoid confusion. - The delivery image destinations include
docker.io/pingcap/br-federation-managerandgcr.io/pingcap-public/dbaas/br-federation-manager. Confirm these repositories are correctly provisioned and authorized.
- The new image is consistently named
Summary of Actionable Suggestions
# delivery.yaml: add clarifying comment on tags_regex
- description: delivery the version images, except pre release types.
# Excludes tags with '-pre' suffix; adjust regex if other pre-release tags exist
tags_regex:
- ^(v[0-9]+[.][0-9]+[.][0-9]+)(-(?!pre(\.|$)).*)?$# packages.yaml.tmpl: add comment describing br-federation-manager image purpose
- name: container image - br-federation-manager
type: image
# Image for br-federation-manager component used in v1.x releases
artifactory:
repo: "{{ .Release.registry }}/pingcap/tidb-operator/images/br-federation-manager"
build_args:
- TARGETARCH={{ .Release.arch }}
context: images/br-federation-manager
dockerfile: Dockerfile- Verify Dockerfile supports multi-arch build with TARGETARCH.
- Confirm CI pipeline integration for building and delivering this image.
- Review regex for pre-release tag filtering for completeness.
Implementing these suggestions will improve clarity, robustness, and maintainability of the packaging and delivery process for br-federation-manager.
![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 packaging and delivery configurations for the br-federation-manager image for tidb-operator v1.x. The changes in packages/packages.yaml.tmpl correctly add the image build for versions below 2.0.0. However, the delivery rule in packages/delivery.yaml is too broad. I've added a comment with a suggestion to make it more specific to v1.x versions to match the build configuration.
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 packaging and delivery support for the br-federation-manager container image version v1.x in the packages.yaml.tmpl and delivery configuration in delivery.yaml. The approach integrates this new image alongside existing components by adding new image build and delivery rules with appropriate regex filtering for version tags. Overall, the changes are straightforward and consistent with the existing patterns, but a few improvements and clarifications are needed for robustness and maintainability.
Code Improvements
-
Regex clarity and correctness in
delivery.yaml(lines ~160-170):The
tags_regexpattern:^(v1[.][0-9]+[.][0-9]+)(-(?!pre(\.|$)).*)?$is intended to exclude pre-release tags but is slightly complex and might miss some edge cases or become hard to maintain.
Suggestion:
- Add a comment explaining the regex intent and examples of matched and excluded tags.
- Consider simplifying or explicitly listing allowed suffixes if possible.
- Verify with test cases that tags like
v1.2.3-alpha,v1.2.3-beta,v1.2.3-pre,v1.2.3behave as expected.
-
YAML duplication in
packages.yaml.tmpl(lines ~1654-1670 and 1698-1712):The
br-federation-managerimage build block is duplicated for two version constraints:- name: container image - br-federation-manager type: image artifactory: repo: "{{ .Release.registry }}/pingcap/tidb-operator/images/br-federation-manager" build_args: - TARGETARCH={{ .Release.arch }} context: images/br-federation-manager dockerfile: Dockerfile
Suggestion:
- Extract this block into a reusable template or anchor to avoid duplication and reduce maintenance overhead.
- If templating language supports it, use variables or conditions to avoid copy-paste.
-
Build context and Dockerfile path verification:
The build context is
images/br-federation-managerwithDockerfileat that location. Ensure this path exists and contains the expected Dockerfile, or the build will fail.Suggestion:
- Double-check the repository to confirm this path.
- Add a comment or documentation about the Dockerfile location for future maintainers.
Best Practices
-
Lack of PR description:
The PR description is empty, which makes it harder for reviewers and future readers to understand the scope and reasoning.
Suggestion:
- Add a concise description summarizing the purpose, components changed, and testing performed.
-
Documentation about the image:
There is no description or documentation about what
br-federation-managerdoes, either in the delivery rules or packages template.Suggestion:
- Add a short descriptive comment in
delivery.yamlandpackages.yaml.tmplexplaining the purpose of this image and any special considerations.
- Add a short descriptive comment in
-
Testing and validation:
No information about testing (e.g., CI jobs, manual tests) is provided.
Suggestion:
- Ensure the new image build and delivery steps are covered in CI pipelines.
- Add tests or validation steps for the regex and delivery correctness if applicable.
Minor Suggestions
-
In
delivery.yaml, the comment line:- description: delivery the version images, except pre release types.
should be corrected to:
- description: Deliver version images, excluding pre-release types. -
Naming consistency: The key
br-federation-manageris consistent. Confirm that this matches the image naming conventions across the repo.
Summary of action items:
- Add a meaningful PR description explaining the change.
- Add comments explaining the regex pattern and tag filtering in `delivery.yaml`.
- Remove duplication of the `br-federation-manager` image build block in `packages.yaml.tmpl` via templating or anchors.
- Confirm Dockerfile path `images/br-federation-manager/Dockerfile` exists and document it.
- Add brief documentation about the new image in both YAML files.
- Verify and add tests or validation for image build and delivery.
- Fix minor grammar in comments for clarity.Addressing these points will improve maintainability, clarity, and reliability of the packaging and delivery process for the new image.
|
@csuzhangxc: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csuzhangxc, 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 |
No description provided.