refactor tiflash-scripts pipeline scripts

[lybcodes]

No description provided.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dillon-zheng for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• pipelines/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary:
This PR refactors parameter initialization in two TiFlash pipeline scripts (pull_regression_test and pull_schrodinger_test). It simplifies the logic by removing conditional checks on input parameters and hardcoding default values instead. The approach drastically reduces complexity but at the cost of removing flexibility and potentially breaking existing behavior that depended on input parameters. The code quality is improved in terms of conciseness, but the functional impact needs careful consideration.

---

Critical Issues

• Behavioral change by removing parameter overrides
  • Files:
    • pull_regression_test/pipeline.groovy (lines 26-51)
    • pull_schrodinger_test/pipeline.groovy (lines 26-61)
  • Issue: The original code allowed parameters like branch, version, testcase, and maxRunTime to be passed in and adjusted dynamically (including special logic for certain branch names). The refactor removes all parameter fetching and conditional handling, always setting fixed default values. This will break pipelines that rely on customized inputs or the previous branch mapping logic.
  • Suggestion: Restore parameter fetching via params.getOrDefault(...) and the conditional logic that ensures backward compatibility and flexibility. If simplification is desired, consider extracting common logic to a shared function but keep parameter handling intact.

---

Code Improvements

• Remove redundant string interpolation for env assignment

  • Files:
    • pull_regression_test/pipeline.groovy (lines 39-41)
    • pull_schrodinger_test/pipeline.groovy (lines 40-43)
  • Issue: Assignments like env.TEST_BRANCH = "${branch}" unnecessarily interpolate a string variable only to assign it back as a string.
  • Suggestion: Assign variables directly without interpolation:

    env.TEST_BRANCH = branch
    env.TEST_VERSION = version
    env.TEST_CASE = testcase
    env.TEST_MAX_RUNTIME = maxRunTime

• Consistent trimming of strings

  • Files:
    • Both pipeline scripts
  • Issue: The refactored code only trims branch but not other parameters like testcase or maxRunTime (even though they are hardcoded here). If these come from params in the future, trimming would be beneficial to avoid whitespace bugs.
  • Suggestion: When restoring parameter fetching, apply .trim() consistently.

---

Best Practices

• Lack of PR description and rationale

  • File: N/A
  • Issue: The PR description is empty, offering no explanation of why this refactor was done or what the intended improvements are. This makes code review and future maintenance more difficult.
  • Suggestion: Add a detailed PR description explaining the motivation, what was simplified, and any known impacts on functionality.

• Testing coverage not mentioned

  • File: N/A
  • Issue: No indication whether these pipeline changes were tested, especially given the removed conditional logic.
  • Suggestion: Ensure that these pipelines are tested in CI with various parameter inputs to confirm no regressions.

• Naming and consistency

  • File: Both pipeline scripts
  • Issue: The use of desc was removed from the description line but left as a hardcoded string. If descriptive messages need to be dynamic, consider keeping a parameter for it.
  • Suggestion: Either remove desc entirely or allow it as an optional parameter with a default value.

---

Summary of actionable suggestions:

// Restore parameter fetching with defaults and trimming
def branch = params.getOrDefault("branch", (REFS.base_ref ?: "master")).trim()
def version = params.getOrDefault("version", "latest").trim()
def testcase = params.getOrDefault("testcase", "schrodinger/bank").trim()
def maxRunTime = params.getOrDefault("maxRunTime", "120").trim()

// Retain special branch logic if needed
if (branch in ["planner_refactory", "raft"]) {
    branch = "master"
}

// Assign env vars directly
env.TEST_BRANCH = branch
env.TEST_VERSION = version
env.TEST_CASE = testcase
env.TEST_MAX_RUNTIME = maxRunTime

// Keep description dynamic or remove desc param consistently
def desc = params.getOrDefault("desc", "TiFlash regression test")
currentBuild.description = "${desc} branch=${branch} version=${version} testcase=${testcase}"

This approach preserves prior functionality and flexibility while still cleaning up the code.

[gemini-code-assist]

Code Review

This pull request simplifies the parameter initialization in the TiFlash regression and schrodinger test pipelines by hardcoding values for branch, version, testcase, and runtime, while removing the ability to override them via Jenkins parameters. The review feedback highlights that these changes eliminate the flexibility needed for manual overrides and break downstream logic, specifically regarding the 'planner_refactory' branch mapping and the accessibility of certain test scenarios like 'sqllogic'. It is recommended to revert to using parameter-based defaults to maintain pipeline functionality.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary
This PR refactors the parameter initialization logic in two pipeline Groovy scripts (pull_regression_test/pipeline.groovy and pull_schrodinger_test/pipeline.groovy) for the TiFlash scripts. It simplifies the handling of parameters by removing conditional overrides and default value checks, instead directly setting fixed defaults or values from the REFS map. The change reduces complexity but also removes some dynamic behavior. The update is straightforward but may impact flexibility and build metadata.

---

Critical Issues

• Loss of dynamic branch selection and parameter overrides

  • Files:

    • pipelines/pingcap-inc/tiflash-scripts/latest/pull_regression_test/pipeline.groovy (lines ~26-50)
    • pipelines/pingcap-inc/tiflash-scripts/latest/pull_schrodinger_test/pipeline.groovy (lines ~26-60)

  • Why: The original code allows parameters like branch, version, testcase, maxRunTime to be overridden via params or ghprbTargetBranch and applies logic to fallback or rewrite certain branches. The refactor removes all of these, hardcoding values and ignoring overrides. This reduces flexibility and may break expected behaviors in the CI flow.

  • Suggested fix: Reintroduce at least minimal support for params or ghprbTargetBranch overrides if they are relied upon by users or other pipeline steps. For example:

    def branch = params.get("branch") ?: (REFS.base_ref ?: "master")
    if (params.ghprbTargetBranch) {
        branch = params.ghprbTargetBranch
    }
    if (branch in ["planner_refactory", "raft"]) {
        branch = "master"
    }
    branch = branch.trim()

    This preserves override capability and legacy branch substitutions.

• Removed build description updates

  • Files: same as above

  • Why: The original pipelines set currentBuild.description with useful info about the test run (branch, version, testcase). This metadata helps traceability in Jenkins UI. The refactor removes these lines entirely.

  • Suggested fix: Restore the build description update with the relevant parameters, for example:

    currentBuild.description = "TiFlash regression test branch=${branch} version=${version}"

    or for schrodinger test:

    currentBuild.description = "TiFlash schrodinger test branch=${branch} version=${version} testcase=${testcase}"

---

Code Improvements

• Use of consistent parameter defaulting

  • Files: both scripts, lines ~26-50 and ~26-60

  • Why: The refactor simplifies parameter setting but uses hardcoded defaults without any defensive trimming or null checks. For better robustness, parameters should be trimmed and defaulted consistently to avoid empty strings.

  • Suggested improvement:

    def version = (params.version ?: "latest").trim()
    if (!version) {
        version = "latest"
    }

• DRY principle for common logic

  • Both scripts implement very similar parameter handling logic (branch, version, etc.)
  • Suggested improvement: Extract common parameter parsing into a shared function or external Groovy library to avoid duplication and ease future maintenance.

---

Best Practices

• Add comments explaining rationale of parameter defaults and overrides

  • Files: both scripts, around parameter initialization

  • Why: Future maintainers will benefit from understanding why certain branches are replaced with "master" or why certain defaults are chosen.

  • Suggested addition:

    // Use target branch if specified, fallback to base_ref or master.
    // Override "planner_refactory" and "raft" branches to master due to compatibility issues.

• Testing coverage for refactored pipeline scripts

  • It's unclear if these pipelines have automated tests to ensure parameter handling works as expected. Consider adding tests or validating in a staging environment.

• Code style: consistent spacing around variable assignments

  • Minor: Add spaces around = for readability:

    def branch = (REFS.base_ref ?: "master").trim()

---

Summary of action items:

• Restore parameter overrides from params and ghprbTargetBranch.
• Restore currentBuild.description updates for visibility.
• Add defensive trimming and null checks on parameters.
• Consider DRY refactoring for shared parameter logic.
• Add explanatory comments on parameter logic.
• Verify pipeline behavior with these changes in test runs.

[ti-chi-bot]

@lybcodes: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-replay-jenkins-pipelines	6c0aab5	link	false	/test pull-replay-jenkins-pipelines

Full PR test history. Your PR dashboard.

Details

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. I understand the commands that are listed here.