refactor(jobs): migrate pingcap-inc/tiflash-scripts to the job-folder layout - #5262
Conversation
|
Skipping CI for Draft Pull Request. |
wuhuizuo
changed the title
e19606a to
0164483
Compare
0164483 to
b970cce
Compare
b970cce to
c3495ae
Compare
c3495ae to
22c09dd
Compare
22c09dd to
a57de74
Compare
![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 pingcap-inc/tiflash-scripts Jenkins jobs by migrating them from the legacy jobs/ and pipelines/ folder layout into the new standardized jenkins/jobs/ folder-per-job layout. The approach involves mostly renaming and relocating files, updating references to job scripts, pod templates, and helper shell scripts to use repo-root-relative paths, and preserving backward compatibility with symlinks. The changes are mechanical and consistent, and validation scripts have been run to ensure correctness. Overall, the PR is well-structured and clean, with no evident logic changes or regressions.
Critical Issues
- None found. The migration is straightforward with no new logic introduced.
Code Improvements
-
Improve path reference consistency:
- In the Jenkinsfiles (e.g.,
pull_regression_test/Jenkinsfileandpull_schrodinger_test/Jenkinsfile), the pod template and helper script paths are now root-relative which is good. - However, consider defining a shared variable or constant for the root-relative base path (
jenkins/jobs/pingcap-inc/tiflash-scripts/latest/) to avoid duplication if future changes are needed. This can reduce errors and improve maintainability.
- In the Jenkinsfiles (e.g.,
-
Error handling in shell script invocation:
- The shell invocations in Jenkinsfiles use
set -euxo pipefailwhich is good practice. - Confirm that any scripts called (like
prepare_runtime_and_artifacts.sh) have proper error handling and exit codes to prevent silent failures.
- The shell invocations in Jenkinsfiles use
Best Practices
-
Documentation:
- The PR description is clear and thorough, but consider adding a README.md inside
jenkins/jobs/pingcap-inc/tiflash-scripts/that explains the new folder layout and how to navigate or update jobs in this folder for future maintainers. - Inline comments in Jenkinsfiles or DSL scripts explaining the purpose of key variables (e.g.,
K8S_NAMESPACE,GIT_CREDENTIALS_ID,REFS) would improve readability for new contributors.
- The PR description is clear and thorough, but consider adding a README.md inside
-
Testing coverage:
- Since this is a migration of job definitions, ensure that downstream consumers or automated tests that trigger these jobs are updated accordingly.
- Consider adding or updating integration tests that validate these Jenkins jobs run correctly post-migration.
-
Naming conventions:
- The renaming process adheres well to the new folder layout.
- The use of
dsl.groovyandJenkinsfilein each job folder is consistent with typical Jenkins job-folder layout conventions.
-
Symlink management:
- The PR mentions symlinks are preserved for backward compatibility but these are not shown in the diff.
- Verify symlinks correctly point to the new locations and are included in the repo to avoid broken references.
Summary of suggested actions:
- Consider defining root-relative base path constants in Jenkinsfiles to reduce duplication, e.g.:
```groovy
final BASE_PATH = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest"
final POD_TEMPLATE_FILE = "${BASE_PATH}/pull_regression_test/pod.yaml"-
Add a README.md in
jenkins/jobs/pingcap-inc/tiflash-scripts/describing the folder layout and instructions. -
Add inline comments in DSL and Jenkinsfiles explaining key variables and steps.
-
Confirm that
prepare_runtime_and_artifacts.shand other scripts have robust error handling. -
Ensure symlinks for backward compatibility are verified and committed.
-
Update or add integration tests that exercise these migrated jobs to catch any issues early.
No blockers or functional bugs identified; this PR is safe to merge after addressing minor maintainability improvements.
a57de74 to
e432510
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 pingcap-inc/tiflash-scripts Jenkins jobs and pipelines by migrating them into the new one-folder-per-job layout under jenkins/jobs/. The migration involves renaming and relocating scripts, pipeline definitions (renamed to Jenkinsfile), DSL definitions (renamed to dsl.groovy), and pod templates, as well as updating path references accordingly. The approach follows a scripted, mechanical migration with back-compat symlinks maintained and validation scripts run. Overall, the changes appear consistent and thorough with no obvious regressions introduced by the path updates.
Critical Issues
- No critical bugs or broken functionality detected.
The primary changes are file moves and path updates. Validation via.ci/check-jenkins-job-references.shpassed as per PR description, indicating references are consistent.
Code Improvements
-
Repeated hard-coded path strings for relocated files
Across multiple Jenkinsfiles and dsl.groovy files, the paths to pod templates and shared scripts are updated with hard-coded strings like:final POD_TEMPLATE_FILE = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml" bash "${WORKSPACE}/jenkins/jobs/pingcap-inc/tiflash-scripts/latest/common/prepare_runtime_and_artifacts.sh" regression
This pattern is repeated in multiple places.
Suggestion:
Consider defining a shared base path variable (e.g.,BASE_JOB_PATH) or utility method for these paths to reduce duplication and ease future migrations or path changes. For example:final BASE_JOB_PATH = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest" final POD_TEMPLATE_FILE = "${BASE_JOB_PATH}/pull_regression_test/pod.yaml" ... bash "${WORKSPACE}/${BASE_JOB_PATH}/common/prepare_runtime_and_artifacts.sh" regression
This will improve maintainability.
-
No error handling changes noted
Since this is a migration/refactor PR, error handling is unchanged. However, verifying that the shell scripts and Jenkinsfiles have appropriate error handling (e.g.,set -euxo pipefailis used) is good. This is already present, so no action needed here.
Best Practices
-
Comments and documentation
The Jenkinsfiles and DSL scripts lack inline comments explaining the key parameters or steps. While this is typical for pipeline code, adding brief comments for constants such asK8S_NAMESPACE,GIT_CREDENTIALS_ID, or the purpose ofprepare_runtime_and_artifacts.shusage would improve readability for future maintainers. For example:// Kubernetes namespace for running the job's pods final K8S_NAMESPACE = "jenkins-tiflow"
-
Testing coverage
The PR description mentions running.ci/check-jenkins-job-references.sh --quietfor validation, but no mention of running or updating any unit or integration tests for these Jenkins jobs. While pipeline jobs are often tested by replay or dry-run, it would be beneficial to ensure that these migrated jobs have at least smoke tests or are verified in staging environments before production promotion. -
Naming conventions
The renamed files follow the new folder layout and naming conventions (e.g.,pipeline.groovy→Jenkinsfile, job DSL scripts renamed todsl.groovy), consistent with the project standard. No issues here. -
Avoid duplication of symlink maintenance
The PR mentions leaving oldjobs/andpipelines/paths as relative back-compat symlinks. It's important these symlinks are verified and maintained by automation to avoid drift over time.
Summary of actionable improvements:
- Refactor repeated hard-coded path strings in Jenkinsfiles and DSL scripts into shared variables for maintainability (e.g.,
BASE_JOB_PATH). - Add brief comments on pipeline constants and script usage for clarity.
- Ensure the migrated jobs have testing or staging validation beyond reference checks.
- Maintain automation for symlink upkeep to avoid manual errors.
These changes will make the migration more robust and easier to maintain long-term.
e432510 to
de3a6de
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 successfully migrates the pingcap-inc/tiflash-scripts Jenkins jobs from a flat structure into the new one-folder-per-job layout under jenkins/jobs/<org>/<repo>/<branch>/<job>/. The migration involves moving DSL, pipeline, and pod template files and updating all references accordingly. The approach correctly uses move semantics and rewrites script paths and pod template references to the new layout. Overall, the changes appear systematic and consistent, with validation scripts passing and staging syntax checks done, indicating a high-quality migration.
Critical Issues
- None found. The PR performs mostly file moves and path updates, and all references seem consistently updated.
Code Improvements
-
Avoid hardcoding repeated path prefixes in Jenkinsfiles and DSL files
In multiple DSL files (e.g.,pull_regression_test/dsl.groovyandpull_schrodinger_test/dsl.groovy), the newciGroovyPathvariable is introduced to avoid duplication, but the pod template paths in Jenkinsfiles remain hardcoded. For consistency and easier future refactoring, consider defining variables for pod template paths as well, for example:final POD_TEMPLATE_FILE = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml"
This pattern is already used, but if pods are referenced elsewhere, centralizing paths reduces brittle code.
-
Parameterize workspace-relative script invocation
In Jenkinsfiles, theprepare_runtime_and_artifacts.shscript is invoked via hardcoded paths relative to${WORKSPACE}:bash "${WORKSPACE}/jenkins/jobs/pingcap-inc/tiflash-scripts/latest/common/prepare_runtime_and_artifacts.sh" regressionTo improve maintainability, consider passing this common base path as an environment variable or parameter, or define a variable in the pipeline script for the common directory, so path changes require less code update.
-
Add error handling for missing files or failed script executions in shell steps
Currently, shell steps useset -euxo pipefail, which is good, but any failure in theprepare_runtime_and_artifacts.shscript will abort the job. This is expected, but consider adding explicit error messages or retry logic if the script is known to occasionally fail due to transient issues.
Best Practices
-
Add file headers or comments to the relocated files indicating their new location and purpose
Since files are moved deeply into a new hierarchy, adding a short comment header in Jenkinsfiles, DSL files, and shell scripts explaining the job folder layout and referencing the migration could help future maintainers. -
Update or add tests to cover the new folder structure usage
The PR description mentions.ci/check-jenkins-job-references.shpasses and staging validation is done, but no tests are added or updated explicitly. Consider adding a job invocation test or linting step that verifies the correctness of the new paths and job triggering, to catch regressions early. -
Consistent naming for DSL files
The DSL files are renamed with.groovyextension but named asdsl.groovy, which is good. Make sure all DSL files follow this convention for consistency, and consider adding a README or documentation in the new folder explaining the purpose of each file. -
Style and naming consistency
- Use double quotes consistently for Groovy strings that include variables (already mostly done).
- The variable
ciGroovyPathis introduced in DSL files—consider naming it more explicitly, e.g.,jenkinsfilePathorpipelineScriptPathfor clarity.
No blocking issues detected; the PR appears well-executed and ready for merge after minor improvements and documentation additions.
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.
de3a6de to
a298a69
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 pingcap-inc/tiflash-scripts Jenkins jobs by migrating all job DSLs, pipelines, and pod templates into the new jenkins/jobs/<org>/<repo>/<branch>/<job>/ folder layout. It updates all relevant scriptPath and pod template references accordingly. The PR primarily consists of renaming and moving files, along with minor path adjustments inside Groovy pipeline and DSL files. Overall, the changes follow a consistent pattern and appear well-structured. Validation steps are described and passed, indicating the move is low risk if all references are correctly updated.
Critical Issues
- No critical bugs or broken functionality observed. The migration is mechanical and references are updated consistently.
Code Improvements
-
Hardcoded path strings repeated in DSL files
Indsl.groovyfiles (e.g.,pull_regression_test/dsl.groovylines 2-6), you define a variable for the scriptPath (ciGroovyPath), which is good. However, the pod template file in the Jenkinsfiles still uses hardcoded strings like:final POD_TEMPLATE_FILE = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml"
This duplication can lead to inconsistencies if paths change again.
Suggestion: Centralize these path strings in a shared config or constants file or pass them as parameters to reduce duplication.
-
Use of double quotes for Groovy strings
Groovy supports both single and double quotes; generally, single quotes are preferred for literals without interpolation for consistency and slight performance benefit. Here:final POD_TEMPLATE_FILE = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml"
could be:
final POD_TEMPLATE_FILE = 'jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml'
This is minor but improves style consistency.
Best Practices
-
Documentation and comments
The DSL files contain a reference comment to the Job DSL API viewer, but the Jenkinsfiles lack comments explaining the purpose of thePOD_TEMPLATE_FILEor other constants. Adding brief comments would improve maintainability, especially in a refactor involving path changes. -
Testing coverage
The PR mentions.ci/check-jenkins-job-references.shpasses and staging validation was done, but no explicit mention of automated test coverage or unit tests for the DSL or pipeline scripts is made. While Jenkins job DSLs are hard to unit test traditionally, consider adding scripted tests or integration tests for job generation if feasible. -
Naming conventions
TheciGroovyPathvariable name is clear, but consider naming it more explicitly, e.g.,pipelineScriptPathorjenkinsfilePath, to clarify what the path points to.
Minor Notes
-
The migration script (
scripts/migrate-jenkins-jobs.sh) is referenced but not included in the diff. Ensure it is well-documented and idempotent to prevent issues with repeated runs. -
The PR description is comprehensive and clearly states the migration approach, which is helpful.
Summary of actionable suggestions
- In DSL files (e.g., jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/dsl.groovy line 2-6):
- Consider centralizing path strings like `ciGroovyPath` and pod template paths to avoid duplication and inconsistencies.
- Rename `ciGroovyPath` to a more descriptive name like `pipelineScriptPath` for clarity.
- In Jenkinsfiles (e.g., pull_regression_test/Jenkinsfile line 5):
- Use single quotes for literal strings to improve consistency:
```groovy
final POD_TEMPLATE_FILE = 'jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pod.yaml'
```
- Add brief comments explaining constants like `POD_TEMPLATE_FILE` for maintainability.
- Consider adding or documenting existing automated tests (unit or integration) for job DSL scripts to ensure migrations do not break job generation.
- Verify the migration script `migrate-jenkins-jobs.sh` has sufficient error handling and is idempotent for safe repeated runs.This PR is well-structured and low risk overall; these improvements will enhance maintainability and future-proof the migrated layout.
|
[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 job configuration by migrating the pingcap-inc/tiflash-scripts jobs from a legacy layout to a new one-folder-per-job structure under jenkins/jobs/<org>/<repo>/<branch>/<job>/. The migration updates DSL, pipeline, and pod template paths accordingly and introduces a ciGroovyPath variable to centralize the Jenkinsfile path reference. The changes mostly involve file renames and path updates. The overall quality is good, with clear path updates and validation steps mentioned. The migration approach is systematic and consistent.
Code Improvements
-
Consistency in
ciGroovyPathusage:-
Files:
jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/dsl.groovy(line 2-3)jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_schrodinger_test/dsl.groovy(line 2-3)
-
Issue: The
ciGroovyPathvariable is declared as afinalstring literal without interpolation or path normalization. If later jobs or tooling expect a normalized path or use this variable in different contexts, inconsistency might arise. -
Suggestion: Consider using a consistent path-building method or constants for the base path prefix, for example:
final basePath = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest" final ciGroovyPath = "${basePath}/pull_regression_test/Jenkinsfile"
This improves maintainability if folder structures change again.
-
-
Hardcoded workspace paths in pipeline scripts:
-
Files:
jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/Jenkinsfile(line ~70)jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_schrodinger_test/Jenkinsfile(line ~80)
-
Issue: The pipeline scripts use hardcoded
${WORKSPACE}/jenkins/jobs/...paths in their shell commands to invokeprepare_runtime_and_artifacts.sh. If the workspace root or job folder changes, these scripts will break. -
Suggestion: Consider defining a shared environment variable or Jenkins parameter for the base job folder path, or compute the path dynamically in Groovy and pass it to the shell environment. For example:
env.JOB_BASE_PATH = "${WORKSPACE}/jenkins/jobs/pingcap-inc/tiflash-scripts/latest/common" sh """ bash "${JOB_BASE_PATH}/prepare_runtime_and_artifacts.sh" regression """
This reduces duplication and the risk of path errors.
-
Best Practices
-
Missing comments on the
ciGroovyPathvariable:-
Files:
jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/dsl.groovy(line 2)jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_schrodinger_test/dsl.groovy(line 2)
-
Issue: The new
ciGroovyPathvariable is introduced but lacks a descriptive comment explaining its purpose and usage pattern, which is important for maintainers unfamiliar with the refactor. -
Suggestion: Add a concise comment, e.g.:
// Path to the Jenkinsfile for this job, used in scriptPath for pipelineJob definition. final ciGroovyPath = "jenkins/jobs/pingcap-inc/tiflash-scripts/latest/pull_regression_test/Jenkinsfile"
-
-
Test coverage validation is only mentioned in PR description:
-
Issue: The PR mentions
.ci/check-jenkins-job-references.shand staging syntax validation but does not include or update any automated tests or validation scripts in the diff. -
Suggestion: If possible, include or update unit or integration tests that verify the new folder layout is correctly handled by the job DSL generation or Jenkinsfile loading. This ensures future refactors won't regress the layout handling.
-
-
File naming consistency:
-
Files renamed from
pipeline.groovytoJenkinsfileunder the new hierarchy. -
Best practice: This is a good move, but ensure that all tooling and documentation referring to pipeline scripts are updated accordingly to prevent confusion.
-
Critical Issues
No critical bugs, security issues, or broken functionality were detected in this migration PR. The changes are mostly file moves and path updates, with validation scripts passed as per the description.
Summary of suggested actionables:
- Declare a shared base path variable for job folder references to improve maintainability.
- Refactor hardcoded workspace paths in shell commands to use environment variables or parameters.
- Add descriptive comments explaining
ciGroovyPathusage. - Consider adding or updating automated tests for the new job layout.
- Confirm all references outside this PR (docs, tooling) are updated to reflect renamed pipeline files (
Jenkinsfile).
Addressing these will improve maintainability and reduce risk of path-related errors in future changes.
|
@wuhuizuo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
1 participant
What
Migrate the
pingcap-inc/tiflash-scriptsslice 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.