refactor(tekton/tikv/migration): use tekton to replace jenkins job migration-docker-release#4287
Conversation
Signed-off-by: lyb <yebin.li@pingcap.com>
for more information, see https://pre-commit.ci
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 refactors the CI/CD pipeline for the Highlights
Changelog
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 refactors the migration Docker release process for the TikV migration repository by replacing the existing Jenkins job with a Tekton trigger and pipeline. The approach introduces a new Tekton Trigger resource that listens for GitHub branch push events to the tikv/migration repo, specifically on the main branch, and then launches a Tekton pipeline to build the component. The changes are concise and focused on Tekton YAML definitions, with no complex logic introduced. Overall, the implementation is clear and aligns with Tekton best practices, but there are some improvements around documentation, maintainability, and validation that could be addressed.
Code Improvements
-
Missing newline at end of file
- File:
tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml(last line) - Why: POSIX standards recommend ending files with a newline to avoid issues with diff tools and some parsers.
- Suggestion: Add a newline at the end of the file.
- File:
-
Filter expression readability and robustness
- File:
git-push.yaml, lines 13-17 - Why: The CEL filter expression is embedded in a multi-line string which can be error-prone and harder to maintain. Also, the regex
^refs/heads/main$is very specific; if other branches need to trigger the pipeline in the future, this would require edits. - Suggestion:
- For readability, consider writing the CEL expression in one line or use indentation consistent with YAML multiline strings.
- If appropriate, parameterize the branch filter for easier future extension.
Example:
value: "body.repository.full_name == 'tikv/migration' && body.ref.matches('^refs/heads/main$')"
- File:
-
Hardcoded values in bindings
- File:
git-push.yaml, lines 20-27 - Why: Values such as
component: migration,os: linux,profile: release,source-ws-size: 20Gi, andtimeout: 60mare hardcoded. While this may be intended, consider if these should be parametrized or documented for future maintainers. - Suggestion: Add comments or consider extracting these values as parameters or config maps if they might change.
- File:
Best Practices
-
Lack of documentation/comments
- Files: Both YAML files, especially
git-push.yaml - Why: The trigger YAML lacks comments explaining the purpose of the trigger, the meaning of each binding, and the role of the interceptor. This can hinder onboarding and future maintenance.
- Suggestion: Add concise comments describing:
- What this trigger does (e.g., "Trigger pipeline on push to main branch of tikv/migration repo")
- Role of each binding (e.g., "component specifies the component to build")
- Purpose of the interceptor filter
For example:
spec: # Interceptor filters events to only trigger on pushes to tikv/migration main branch interceptors: - name: filter on repo and branches ...
- Files: Both YAML files, especially
-
Testing and validation
- Why: No evidence of testing or validation steps for the new Tekton trigger. Since this replaces a Jenkins job, it’s important to verify all expected behaviors.
- Suggestion: Include or link to test plans or pipelines runs that validate this trigger works as expected, or add unit tests for CEL expressions if supported.
-
Kustomization.yaml update confirmation
- File:
kustomization.yaml, line 34 - Why: The new resource
tikv/migration/git-push.yamlwas added to the kustomization.yaml. Verify that this is the intended place and that the resource path matches your repo structure. If multiple environments exist, consider whether this should be environment-specific. - Suggestion: Confirm and document the choice of placing it under
env-gcp.
- File:
Summary of Actionable Suggestions
- Add a newline at end of `git-push.yaml`.
- Simplify and/or document the CEL filter expression for better readability and future extensibility.
- Add comments to `git-push.yaml` explaining the purpose of the trigger, interceptor, and bindings.
- Consider parameterizing hardcoded values or at least document them.
- Ensure testing/validation steps for the new Tekton trigger are in place.
- Confirm the kustomization resource path and document the rationale.
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 refactors the CI/CD setup by replacing the Jenkins job migration-docker-release with a Tekton pipeline trigger for the tikv/migration repository. The approach adds a new Tekton Trigger resource under env-gcp that listens to GitHub branch push events specifically on the main branch of the tikv/migration repo. The overall code quality looks clean and consistent with existing Tekton trigger definitions.
Code Improvements
-
Trigger Filtering Logic (File:
tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml, lines 13-17)
The CEL filter expression uses amatchesfunction onbody.refto match the branch name. However,body.refis the full ref string (e.g.,refs/heads/main). The regex^refs/heads/main$works but could be simplified or made more readable.
Suggestion:
Use exact string comparison if only themainbranch is needed:value: "body.repository.full_name == 'tikv/migration' && body.ref == 'refs/heads/main'"
This approach is more performant and less error-prone than regex matching.
-
Timeout Parameter (File:
tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml, lines 26-27)
The timeout is set to60m, which may be sufficient but depends on the migration build duration. Consider making this configurable or documenting the rationale.
Suggestion: Add a comment or link to documentation explaining why 60 minutes is chosen or provide a variable for easier updates. -
Parameter Consistency
The trigger passes parameters likecomponent,os,profile,source-ws-size, andtimeout. Verify that these parameters align with thebuild-component-linuxtemplate and that all required parameters are passed. Missing or extra params could cause runtime errors.
Suggestion: Cross-checkbuild-component-linuxfor expected parameters.
Best Practices
-
Documentation (Files: All new Tekton trigger files)
There is no inline comment or external documentation referencing this new trigger. Since this replaces an existing Jenkins job, it would be helpful to add:- A brief comment explaining the purpose of this trigger and its relation to the Jenkins job it replaces.
- Documentation updates outside of code (e.g., README or Confluence) to guide maintainers on the migration from Jenkins to Tekton.
Example comment at the top of
git-push.yaml:# Tekton Trigger for tikv/migration repository. # Replaces the Jenkins job 'migration-docker-release' with a Tekton-based build trigger.
-
Testing Coverage
There's no indication of tests or validation for this trigger configuration. While Tekton triggers are declarative, it is good practice to have integration tests or manual validation steps documented to ensure the trigger behaves as expected on GitHub events. -
Naming Conventions
The trigger namegit-push-tikv-migrationis clear and consistent. No issues found here.
Critical Issues
No critical issues such as bugs, security vulnerabilities, or broken functionality were identified from the provided diff. The changes are additive and focused on configuration.
Summary of Actionable Suggestions:
- Simplify the CEL filter expression for branch matching to exact string comparison.
- Document or justify the 60-minute timeout value.
- Verify parameter alignment with the referenced Tekton template
build-component-linux. - Add comments and documentation about the trigger’s purpose and Jenkins replacement.
- Consider adding or documenting testing steps for this new trigger.
![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 migrates the "migration-docker-release" Jenkins job to a Tekton trigger for the "tikv/migration" repository. A critical security vulnerability has been identified: the trigger configuration lacks webhook signature validation, which could allow unauthorized users to trigger builds with malicious source code and lead to a supply chain attack. It is strongly recommended to add a github interceptor for HMAC signature validation. Additionally, there are minor suggestions to improve the overall robustness of the new trigger, including a file formatting convention.
Signed-off-by: lyb <yebin.li@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 refactors the CI pipeline for the tikv/migration repository by replacing the existing Jenkins job migration-docker-release with a Tekton-based trigger and pipeline. The approach introduces a new Tekton Trigger resource that listens for GitHub push events on the tikv/migration repo’s main branch, with relevant pipeline parameters configured for building the component. The changes are concise, focused, and follow Tekton best practices for event filtering and parameterization.
Code Improvements
-
Trigger Filtering Logic (
tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml, lines 11-17):
The CEL expression for filtering GitHub events usesbody.ref.matches('^refs/heads/main$'). While this works, the use ofmatches()with a regex for an exact match is somewhat heavy. Consider usingbody.ref == 'refs/heads/main'for simplicity and improved readability and performance:value: >- body.deleted != true && body.repository.full_name == 'tikv/migration' && body.ref == 'refs/heads/main'
-
Timeout Configuration (
kustomization.yamlandgit-push.yaml):
The timeout is set to"60m"as a string. Verify that the Tekton pipeline and trigger templates expect a string or duration format. If numeric minutes are required, ensure this matches the expected type to avoid silent misconfiguration. -
Resource Naming Consistency (
kustomization.yaml):
You addedtikv/migration/git-push.yamlto the resources list. Please confirm that the file path and naming convention (git-push.yaml) is consistent with other similar triggers to maintain uniformity.
Best Practices
-
Documentation (
git-push.yaml):
The new trigger resource lacks comments describing its purpose and the parameters. Adding brief comments would improve maintainability, for example:# Trigger for building tikv/migration on pushes to the main branch. # Filters out deleted branch events. # Uses build-component-linux template with release profile.
-
Testing Coverage:
There is no mention of test coverage or validation for this new Tekton trigger. Consider adding or referencing tests that simulate GitHub push events to verify the trigger fires correctly and the pipeline runs as expected.
Critical Issues
No critical bugs, security issues, or broken functionality observed in the provided diff.
Overall, the PR is well-scoped and integrates the new Tekton trigger cleanly. Addressing the above improvements will enhance readability, maintainability, and reliability.
tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: lyb <yebin.li@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 refactors the Tekton pipeline setup to replace the Jenkins job for migration-docker-release in the tikv/migration repository. It adds a new Tekton Trigger YAML to listen for GitHub branch pushes on the main branch of tikv/migration and includes it in the GCP environment kustomization. The approach is straightforward, defining a Trigger with relevant bindings and an interceptor filter. Overall, the code is clear and well-structured for the intended purpose.
Code Improvements
-
Trigger Filtering Logic (tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml, lines 11-15)
The CEL filter expression currently hardcodes the repository full name and branch as a single string match:body.repository.full_name == 'tikv/migration' && body.ref.matches('^refs/heads/main$')This is redundant given the trigger is scoped to
tikv/migrationrepo andmainbranch by GitHub webhook configuration.
Suggestion: Simplify the interceptor or ensure this matches production webhook payloads exactly to avoid trigger misses. Alternatively, document why both repo and branch filtering is necessary here. -
Timeout Parameter (lines 27-28)
The timeout is set to60m, which may be appropriate, but consider verifying if this duration aligns with the migration build's expected runtime based on historical Jenkins job data.
Suggestion: If builds frequently take longer, increase timeout or make it configurable. -
Parameter Consistency
The bindings includecomponent: migration,os: linux,profile: release, andsource-ws-size: 20Gi. Confirm these values match the Jenkins job parameters to ensure functional equivalence.
Suggestion: Add a comment referencing the Jenkins job or include a README snippet describing these parameters for maintainability.
Best Practices
-
Documentation (tekton/v1/triggers/triggers/env-gcp/tikv/migration/git-push.yaml)
The new trigger YAML lacks comments explaining its purpose and relationship to the replaced Jenkins job.
Suggestion: Add a header comment, e.g.:# Trigger to start Tekton pipeline on main branch push for tikv/migration, # replacing the Jenkins migration-docker-release job.
-
Testing Coverage
No mention of tests or validation of the new Tekton trigger.
Suggestion: Ensure this trigger is tested in a staging environment with test pushes to verify pipeline execution and parameter correctness. -
Kustomization Inclusion (env-gcp/kustomization.yaml, line 35)
The new resource is added:- tikv/migration/git-push.yamlConfirm that this file path is correct relative to the kustomization root to avoid deployment errors.
Minor Suggestions
- Labeling Consistency
The labeltype: github-branch-pushin the Trigger metadata is generic. Consider aligning labels with other triggers for easier filtering and management.
Addressing the above points will improve maintainability, clarity, and robustness of this Jenkins-to-Tekton migration for the tikv/migration component.
|
[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 |
[LGTM Timeline notifier]Timeline:
|
2 participants
Depends on: PingCAP-QE/artifacts#907
Close: #3347