refactor(jobs): migrate tikv/migration to the job-folder layout - #5260
Conversation
|
Skipping CI for Draft Pull Request. |
e5d1e09 to
b7b4ff6
Compare
b7b4ff6 to
80c74fd
Compare
80c74fd to
b1a4b7d
Compare
b1a4b7d to
c940019
Compare
c940019 to
87ce972
Compare
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 tikv/migration Jenkins jobs by migrating them to the new one-folder-per-job layout under jenkins/jobs/. It renames and moves job scripts, pod templates, and DSL definitions accordingly, updating all internal script paths to repo-root-relative references. The PR maintains backward compatibility with symlinks and validates references via a script. Overall, the changes are systematic and consistent, reflecting a well-planned migration with minimal logic changes.
Code Improvements
-
Update Hardcoded Paths to Use Variables or Constants
In files likejenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfileand similar ones, the pod template file path is hardcoded as:final POD_TEMPLATE_FILE = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/pod.yaml"
Consider centralizing such paths into constants or utility functions, especially since this migration means other jobs will have similar path structures. This reduces duplication and eases future path changes.
-
Symlink Back-Compatibility Verification
The PR mentions symlinks for backward compatibility but does not show their creation or test coverage. Ensure that:- Symlinks are included in the PR or documented clearly.
- Automated tests verify that old paths still resolve correctly.
- If not yet automated, plan to add such tests to prevent regressions.
-
Consider DRY for Repeated Job Definitions
The DSL files for different jobs are very similar except for job names and paths. To improve maintainability:- Introduce shared Groovy methods or templates to generate similar jobs with parameters.
- This minimizes duplication and helps future migrations or changes.
Best Practices
-
Documentation of Migration Impact
While the PR description is thorough, adding a README or comment in thejenkins/jobs/tikv/migration/folder summarizing the new layout and referencing the migration script would aid future maintainers. -
Testing Coverage
The PR mentions.ci/check-jenkins-job-references.shpasses, which is good. However:- Confirm that Jenkins jobs trigger and execute successfully after move.
- Add or update automated integration tests or smoke tests for these jobs if not already present.
-
Consistent Naming Conventions
The use ofJenkinsfileanddsl.groovyis consistent and clear. Just ensure naming conventions for new jobs follow this established pattern to avoid confusion.
Minor Suggestions
-
In DSL files, consider adding brief comments on what the job does or any unique configuration for easier understanding by new contributors.
-
Confirm that all
finalvariables used in Jenkinsfiles are necessary and used consistently (e.g.,GIT_CREDENTIALS_ID,K8S_NAMESPACE).
Summary of Actions:
- Centralize hardcoded path strings into constants or shared utilities.
- Ensure symlink back-compatibility is fully tested and documented.
- Refactor repetitive DSL job definitions to reduce duplication.
- Add documentation in the new job folder about the layout and migration.
- Verify Jenkins job execution post-migration with integration tests.
- Add brief comments in DSL files for clarity.This PR is a solid structural refactor; addressing these points will improve maintainability and robustness.
87ce972 to
6446ee9
Compare
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 Tikv/migration Jenkins jobs by migrating them from the legacy flat folder layout (jobs/ and pipelines/) to the new one-folder-per-job layout under jenkins/jobs/. It updates job definitions, pod templates, and DSL scripts accordingly, rewriting script paths and pod template references to new repo-root-relative locations. The approach is automated via a migration script with validation checks ensuring reference correctness. The changes are systematic and mostly file moves with necessary path updates. Overall, the PR is well-structured, and no immediate critical issues are apparent.
Code Improvements
-
Path Reference Consistency
In the DSL files (dsl.groovy), thescriptPathis updated to use double quotes, e.g.scriptPath("jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfile")but in the Jenkinsfiles themselves, pod template paths use a mixture of single and double quotes, e.g.
final POD_TEMPLATE_FILE = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/pod.yaml"
For consistency and readability, consider standardizing on either single or double quotes across these files.
-
Hardcoded Namespace and Credentials
The Jenkinsfiles hardcode values like:final K8S_NAMESPACE = "jenkins-tidb" final GIT_CREDENTIALS_ID = 'github-sre-bot-ssh'
If these values are common across many jobs, consider centralizing them in a shared configuration or environment variables to reduce duplication and ease updates.
-
Error Handling for JSON Parsing
The line:final REFS = readJSON(text: params.JOB_SPEC).refs
assumes
params.JOB_SPECis always valid JSON and has a.refsproperty. Add error handling or validation to catch malformed or missingJOB_SPECto prevent pipeline failures. -
Edge Case: Base Ref Fallback
The logic:final COMPONENT_ARTIFACT_BASE_REF = REFS.base_ref == 'main' ? 'master' : REFS.base_ref
assumes
base_refis always present. Consider adding a fallback default or validation to handle unexpected cases wherebase_refmight be null or empty.
Best Practices
-
Documentation and Comments
The migrated Jenkinsfiles and DSL scripts contain no comments explaining the purpose or usage of key variables likeOCI_TAG_TIDB, or the overall job behavior. Adding brief comments will improve maintainability, especially given the complexity of CI pipelines. -
Testing Coverage
The PR description mentions validation by.ci/check-jenkins-job-references.sh, which is good. However, there is no mention of integration or end-to-end tests verifying that the jobs function correctly after migration. Consider adding or updating test cases to cover job execution in the new layout. -
Naming Conventions
The folder and file naming is consistent with the new layout. However, the job names in the DSL (pipelineJob('tikv/migration/pull_integration_kafka_test')) remain unchanged, which is expected for backward compatibility. Just ensure there are no conflicts or ambiguities with other jobs. -
Symlink Back-compatibility
The PR mentions old paths are kept as relative back-compat symlinks. Verify these symlinks are correctly created and maintained in the repository, and document this to avoid confusion for future maintainers.
Summary of Actionable Suggestions
- Standardize quotation marks (
'vs") in Jenkinsfiles and DSL scripts for consistency. - Centralize hardcoded constants like
K8S_NAMESPACEandGIT_CREDENTIALS_IDif possible. - Add error handling around
readJSONusage for safer pipeline execution. - Add comments explaining key variables and job logic in Jenkinsfiles and DSL scripts.
- Ensure test coverage for job execution after migration is adequate.
- Verify and document the creation and maintenance of back-compat symlinks.
Addressing these will improve maintainability, robustness, and clarity of the migrated jobs.
6446ee9 to
a2848a9
Compare
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 tikv/migration Jenkins jobs by migrating them to a new job-folder layout under jenkins/jobs/<org>/<repo>/<branch>/<job>/. The approach involves moving DSL, pipeline Jenkinsfiles, and pod templates into their respective job directories and updating references accordingly (e.g., scriptPath, pod template paths). The changes are mostly renames and path updates. The overall quality appears consistent and systematic, with validation scripts passing and staging validation done. The PR follows a clear migration pattern.
Code Improvements
-
Improve path reference consistency and maintainability
In DSL files (e.g.,jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/dsl.groovyat line 2 onward), you define a local variableciGroovyPathfor the Jenkinsfile path:final ciGroovyPath = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfile"
However, in the corresponding pipeline files (
Jenkinsfile), you use a string literal directly for pod templates, e.g.:final POD_TEMPLATE_FILE = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/pod.yaml"
Issue: This hard-coded string usage can lead to duplication and risk of inconsistency if paths change.
Suggestion: Consider centralizing path constants or using a shared config snippet or method to generate these paths dynamically, reducing duplication and maintenance burden. For example:
def getJobPath(jobName) { return "jenkins/jobs/tikv/migration/latest/${jobName}" } final ciGroovyPath = "${getJobPath('pull_integration_kafka_test')}/Jenkinsfile" final POD_TEMPLATE_FILE = "${getJobPath('pull_integration_kafka_test')}/pod.yaml"
-
Error handling or validation on missing files
The migration changes paths to new locations. If a referenced file is missing or the path is incorrect, Jenkins job runs might fail silently or cause confusing errors.Suggestion: Add sanity checks or stage-level validation that the
Jenkinsfileandpod.yamlexist at the specified new locations. This can be done either in the Jenkins pipeline or in CI validation scripts. This will improve robustness after migration.
Best Practices
-
Documentation update for new layout usage
The code comments still reference old paths (e.g.,pipelines/tikv/migration/latest/pull_integration_kafka_test.groovyin comments). For instance, in DSL files:// REF: https://<your-jenkins-server>/plugin/job-dsl/api-viewer/index.html // For trunk and latest release branches.
Issue: While the comment is generic, it would be helpful to update/add comments indicating the new job folder layout standard, so future maintainers understand the layout rationale and where to find job files.
Suggestion: Add a brief comment near the top of DSL and pipeline files mentioning the new layout and migration status, e.g.:
// This job follows the new one-folder-per-job Jenkins layout as part of the 2024 migration. -
Testing coverage and validation
The PR description mentions validation with.ci/check-jenkins-job-references.shand staging syntax validation. However, there is no indication of adding automated tests for these changes or job behavior verification post-migration.Suggestion: Add or update integration tests or Jenkins replay tests to cover these migrated jobs, ensuring no behavioral regressions. Consider adding a test suite that validates job loading and execution paths after migration.
-
Naming conventions
The new layout uses lowercase folder names matching job names, which is good for consistency. No issues found here.
Critical Issues
- None detected in the diff. The changes appear to be renames and path updates without logic changes.
Overall, this PR effectively refactors the job layout with minimal risk. The key improvements would be around reducing path duplication, adding explicit documentation about the new layout, and enhancing validation or testing around file presence and job correctness post-migration.
The migration flattened every `scriptPath(...)` to a hardcoded path and left
the DSL's own variables (`fullRepo`, `branchAlias`, `jobName`,
`ciGroovyPath`) unused or stale, losing the maintainability the variables
were introduced for. Define/update `final ciGroovyPath = "<path>"` and call
`scriptPath(ciGroovyPath)`.
The value is templated from the DSL's real `final` variables when they
reproduce the target path (e.g.
`"jenkins/jobs/${fullRepo}/${branchAlias}/${jobName}/Jenkinsfile"`), and
falls back to the literal path otherwise (non-standard job names, missing
variables).
Generated with AI assistance.
…licating it When the DSL already declares ciGroovyPath, replace its value; do not also insert a new declaration at the top. Generated with AI assistance.
Generated with AI assistance.
Add an `--apply` pass that rewrites a migrated job's `scriptPath` through `ciGroovyPath` even when its legacy DSL is gone, so jobs migrated before the convention (for example `tikv/copr-test` and the `test-prod` pilot) are brought in line. Generated with AI assistance.
a2848a9 to
9f91791
Compare
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 migrates the tikv/migration Jenkins jobs and related files (DSL, pipeline scripts, pod templates) into a new one-folder-per-job layout under jenkins/jobs/tikv/migration/.... The migration updates script path references and pod template locations accordingly and uses a script to ensure no duplicate legacy files remain. The changes are mostly file renames with path updates and appear systematic and consistent. Overall, the PR looks well-structured and clean, reflecting a solid migration effort.
Critical Issues
- None found. The migration mostly involves renaming and path updates without altering logic.
Code Improvements
-
Hardcoded paths in Jenkinsfiles and DSL:
Files such as
jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfileuse hardcoded strings for pod template paths and script paths:final POD_TEMPLATE_FILE = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/pod.yaml" ... final ciGroovyPath = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfile"
Issue: If the folder structure changes again, these paths require manual updates.
Suggestion: Consider centralizing base path constants or use relative paths where possible to ease future maintenance. For example:
final JOB_BASE_DIR = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test" final POD_TEMPLATE_FILE = "${JOB_BASE_DIR}/pod.yaml" final ciGroovyPath = "${JOB_BASE_DIR}/Jenkinsfile"
-
Script path assignment in DSL files:
DSL files assign the script path to a variable and then use it in
scriptPath(ciGroovyPath):final ciGroovyPath = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/Jenkinsfile" ... scriptPath(ciGroovyPath)
This is generally good, but since the variable is only used once, consider inlining it unless you anticipate adding more logic around it.
-
Potential missing error handling or validation:
While the PR mentions running
.ci/check-jenkins-job-references.shand staging validation, there is no code-level validation for the presence of files or correct paths inside the Jenkinsfiles or DSL scripts.Suggestion: If feasible, add sanity checks or comments to remind maintainers to ensure pod templates and scripts exist at referenced paths.
Best Practices
-
Documentation:
- The DSL files have a brief comment referencing the Jenkins Job DSL API, which is helpful.
- Consider adding a short header comment in each migrated Jenkinsfile and DSL file indicating that the file is auto-generated or migrated, and where to find the source or migration script. This aids future developers in understanding the origin and maintenance process.
-
Naming conventions:
- The naming is consistent and uses clear folder and file names matching the job names.
- The DSL filenames use
.groovysuffix except for the Jenkinsfiles which are renamed properly.
-
Testing coverage:
- The PR description mentions running
.ci/check-jenkins-job-references.shand staging validation but does not mention automated tests. - Consider adding or updating automated tests (if any exist) to cover the new folder layout or ensure backwards compatibility.
- The PR description mentions running
-
Code duplication:
- The migration mainly moves existing files and updates paths. There is some duplication of path strings in Jenkinsfiles and DSL scripts, as noted above.
- If multiple jobs share similar pod templates or scripts, consider referencing shared templates or extracting common code to reduce duplication.
Summary of action items:
- [ ] Refactor hardcoded path strings in Jenkinsfiles and DSL files into centralized constants or use relative paths to ease future maintenance.
- [ ] Add header comments to migrated Jenkinsfiles and DSL files indicating their migration origin and instructions for maintainers.
- [ ] Verify presence of pod template and script files with inline comments or simple validation in Jenkinsfiles.
- [ ] Consider adding or updating automated tests to cover the new folder layout and job definitions.
- [ ] Review potential for shared pod templates or script reuse to reduce duplication.This PR is a solid migration step with minimal risk; focusing on maintainability improvements will maximize long-term benefits.
|
[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 |
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 Jenkins jobs related to tikv/migration by migrating them into a new folder layout, where each job resides in its own directory under jenkins/jobs/<org>/<repo>/<branch>/<job>/. The migration involves moving DSL, pipeline scripts, and pod templates, and updating the job definitions to use a ciGroovyPath variable for the pipeline script path. The approach is systematic and uses tooling and scripts to automate the migration. Overall, the changes appear consistent, with good validation steps and adherence to the new layout. The PR shows good structure but could benefit from some improvements to maintainability and clarity.
Critical Issues
- No critical bugs or broken functionality found in diff.
The renames and path updates appear consistent, and validation scripts pass as stated.
Code Improvements
-
Use of
ciGroovyPathas afinalvariable in DSL files- File(s):
jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test/dsl.groovyandjenkins/jobs/tikv/migration/latest/pull_integration_test/dsl.groovy - Issue: Declaring
ciGroovyPathas a Groovyfinaloutside the job closure is fine, but for clarity and maintainability, it could be declared inside thepipelineJobclosure or passed as a parameter, to avoid potential scoping or reuse confusion. - Suggestion: Encapsulate
ciGroovyPathinside the job closure or add a comment explaining why it is declared globally in the DSL file.
- File(s):
-
Hardcoded strings for Jenkins paths
-
File(s): Same as above
-
Issue: The path strings for
ciGroovyPathand pod templates are hardcoded and duplicated in several places. This can lead to errors if folder structure changes again. -
Suggestion: Consider centralizing path construction using variables or utility methods, or use string interpolation consistently, e.g.:
def basePath = "jenkins/jobs/tikv/migration/latest/pull_integration_kafka_test" final ciGroovyPath = "${basePath}/Jenkinsfile" final POD_TEMPLATE_FILE = "${basePath}/pod.yaml"
-
-
Unify naming and file extensions for pipeline scripts
- File(s): Pipeline scripts renamed from
.groovytoJenkinsfile - Issue: The rename of pipeline scripts to
Jenkinsfileis good for clarity, but the DSL scripts remain.groovy. Ensure this naming convention is documented and consistent across the repo to avoid confusion between DSL and pipeline scripts. - Suggestion: Add a README or comment in the folder explaining the purpose and naming conventions of each file.
- File(s): Pipeline scripts renamed from
-
Error handling in job DSL scripts
-
File(s): DSL files
-
Issue: The DSL scripts do not appear to handle or validate parameters like
params.JOB_SPECbefore use, which could cause runtime errors if parameters are malformed or missing. -
Suggestion: Add defensive checks or try-catch blocks for parameter parsing, e.g.:
def refs try { refs = readJSON(text: params.JOB_SPEC).refs } catch (Exception e) { error("Failed to parse JOB_SPEC parameter: ${e.message}") }
-
Best Practices
-
Add comments explaining the migration and new structure
-
File(s): Top-level DSL and pipeline scripts in
jenkins/jobs/tikv/migration/... -
Issue: The new folder layout and usage of
ciGroovyPathis non-trivial and may confuse maintainers unfamiliar with the migration. -
Suggestion: Add a short header comment in DSL and pipeline files explaining the folder layout, how
ciGroovyPathworks, and referencing the migration script or PR. For example:// This job is part of the tikv/migration Jenkins job migration to the new folder layout. // Pipeline script path is managed via the ciGroovyPath variable for easier refactoring.
-
-
Testing coverage for migrated jobs
- File(s): Not directly visible from patch, but important given migration
- Issue: The PR relies on
.ci/check-jenkins-job-references.shand manual validation; automated tests for Jenkins job definitions or integration tests would strengthen confidence. - Suggestion: If not already present, add or extend tests that validate job DSL correctness, pipeline execution, and pod template validity, possibly with automated replay on staging Jenkins.
-
Consistent quoting style
- File(s): Various Groovy files
- Issue: The patch switches between single and double quotes (e.g.,
final POD_TEMPLATE_FILEuses single quotes in some places, double quotes in others). - Suggestion: Adopt a consistent quoting style per project guidelines (usually double quotes for Groovy strings with interpolation, single quotes otherwise) to improve readability.
-
Avoid duplication of similar job DSL code
-
File(s):
dsl.groovyfiles for each job appear similar -
Issue: Job DSL definitions for similar jobs are almost identical except for job names and paths.
-
Suggestion: Abstract common DSL code into shared methods or templates to reduce duplication, e.g.:
def createPipelineJob(name, ciGroovyPath) { pipelineJob(name) { ... definition { cpsScm { scriptPath(ciGroovyPath) ... } } } } createPipelineJob('tikv/migration/pull_integration_test', 'jenkins/jobs/tikv/migration/latest/pull_integration_test/Jenkinsfile')
-
Overall, this PR performs a meaningful and well-structured migration with minimal risk. Addressing the above improvements would enhance maintainability, reduce duplication, and clarify the new layout for future contributors.
What
Migrate the
tikv/migrationslice of Jenkins jobs to the one-folder-per-job layout,generated by:
The migration moves each job's DSL, pipeline and pod templates into
jenkins/jobs/<org>/<repo>/<branch>/<job>/and rewritesscriptPaththrougha
ciGroovyPathvariable (final ciGroovyPath = "jenkins/jobs/..."+scriptPath(ciGroovyPath)), templated from the DSL's own variables wherepossible. A source shared with a not-yet-migrated job is copied for the
earlier job and moved by the last one, so no legacy duplicate remains.
Stack
Part of the stacked migration for Conductor track
jenkins_job_layout_20260916. Depends on#5291 (
ciGroovyPathrewrite); themove-semantics tooling is already in
mainvia#5258.
Validation
.ci/check-jenkins-job-references.shpasses on this layer.Generated with AI assistance.