[wuhuizuo]

Why

pingcap/tiflash is a public repository, but it contains private submodules
(contrib/tiflash-proxy-next-gen and contrib/cloud-storage-engine point to
git@github.com:tidbcloud/cloud-storage-engine.git). The gcp darwin path runs
through MacBuild, and its clone stage initializes submodules. Without a
source.gitSecretRef, the clone is anonymous and fails with
Permission denied (publickey).

The existing darwin triggers only pass github-pat-secret for
pingcap-inc/tici; the community darwin triggers do not bind git-secret-ref
at all.

What

• For the six fake-github triggers (branch-push / pr / tag-create, both the
  darwin-only and platform=all variants), set git-secret-ref to
  github-ssh-secret when the repository is pingcap/tiflash.
• Keep pingcap-inc/tici on github-pat-secret.
• Add a git-secret-ref overlay and binding to both community darwin triggers
  (github-push-build-community-darwin and the FTS variant), only for
  pingcap/tiflash.
• Add a regression test at
  tekton/tests/darwin-mac-credentials.test.ts.
• Include a one-line resource sort in tekton/v1/pipelines/kustomization.yaml
  required by .ci/update-tekton-kustomizations.sh (pre-existing drift; no
  behavior change).

No template or pipeline parameter contract changes are needed; the existing
git-secret-ref chain is reused.

Validation

• deno test ... tekton/tests/darwin-mac-credentials.test.ts: 1 passed
  (8 steps); the test fails on the old triggers.
• Full Deno suite: 9 passed (92 steps).
• kustomize build tekton/v1/triggers/triggers/env-gcp: exit 0.
• .ci/update-tekton-kustomizations.sh: exit 0 after the sort fix.
• uvx pre-commit run --files <changed files>: passed.
• uvx pre-commit run --all-files still reports pre-existing pod-resource
  policy violations in unrelated Jenkins pod YAMLs; the changed files are clean.

Rollback

Revert this PR; the change is configuration-only.

Notes

The SSH key in github-ssh-secret must be authorized for
tidbcloud/cloud-storage-engine (including tidbcloud SAML SSO). The e2e run
will confirm this; if it fails, the follow-up is a dedicated read-only
credential or the GitHub App token broker.

[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 introduces a fix to handle private Git submodules for the pingcap/tiflash repository in Tekton pipelines for Darwin builds. The solution involves passing a specific git-secret-ref (github-ssh-secret) for pingcap/tiflash in multiple trigger configurations to ensure submodules can be cloned properly. It also includes a regression test (darwin-mac-credentials.test.ts) to validate the new triggers and a minor resource sorting fix in a kustomization.yaml file. Overall, the PR is well-structured and addresses the issue effectively, with good documentation and test coverage. However, there are some areas that could benefit from refinement and additional considerations.

---

Critical Issues

• Security Context for github-ssh-secret:
  • Files: All modified Tekton trigger YAMLs
  • Issue: The PR assumes that the github-ssh-secret is authorized for accessing private submodules. However, this is only mentioned in the "Notes" section of the description, and no mechanism is provided in the code to verify or enforce this authorization. If the secret is not properly configured, the pipeline will fail during execution.
  • Suggested Solution: Add explicit validation or logging in the pipeline tasks to ensure the github-ssh-secret has the required permissions. Consider adding a pre-check step as part of the pipeline to verify SSH access to the submodules.

---

Code Improvements

• Repeated Logic for git-secret-ref Overlay:

  • Files: All modified Tekton trigger YAMLs
  • Issue: The logic for setting the git-secret-ref is repeated across multiple files:

    'git-secret-ref': body.repository.full_name == 'pingcap-inc/tici' ? 'github-pat-secret' : (body.repository.full_name == 'pingcap/tiflash' ? 'github-ssh-secret' : ''),

    This repetition increases maintenance overhead and can lead to inconsistencies if the logic changes.
  • Suggested Solution: Refactor the repeated logic into a reusable CEL function and reference it in all triggers. For example:

    'git-secret-ref': myCustomFunction(body.repository.full_name),

    Define myCustomFunction in a shared interceptor or reusable CEL library.

• Test Utility Refactoring:

  • File: darwin-mac-credentials.test.ts
  • Issue: The test utilities (e.g., assertSecret, overlayExpression, byName) are tightly coupled with this single test file, despite being potentially reusable in other tests.
  • Suggested Solution: Move these utilities to a shared library/module to facilitate reuse across test files. This will reduce duplication and improve maintainability.

---

Best Practices

• Documentation for github-ssh-secret:

  • Files: All modified Tekton trigger YAMLs, and potentially associated README or documentation files.

  • Issue: The purpose and usage of github-ssh-secret are not documented in the Tekton configuration files. This could lead to confusion for future maintainers.

  • Suggested Solution: Add comments in the YAML files explaining why github-ssh-secret is being used. Additionally, update any relevant documentation to include instructions for setting up this secret and ensuring its proper configuration.

    For example:

    # Use github-ssh-secret for accessing private submodules in pingcap/tiflash.
    # Ensure the secret has the necessary permissions to access private repositories.
    'git-secret-ref': body.repository.full_name == 'pingcap-inc/tici' ? 'github-pat-secret' : (body.repository.full_name == 'pingcap/tiflash' ? 'github-ssh-secret' : ''),

• Testing Coverage for Edge Cases:

  • File: darwin-mac-credentials.test.ts
  • Issue: The test does not appear to cover edge cases such as:
    • Repositories other than pingcap/tiflash or pingcap-inc/tici.
    • Invalid or missing git-secret-ref.
  • Suggested Solution: Add test cases for these scenarios to ensure the configuration handles unexpected inputs gracefully.

---

Optional Improvements

• Sorting Fix in kustomization.yaml:
  • File: tekton/v1/pipelines/kustomization.yaml
  • Issue: The sorting change is unrelated to the main fix in the PR, which could complicate rollback or debugging.
  • Suggested Solution: Consider moving this change to a separate PR to maintain better isolation of concerns.

---

Conclusion

This PR addresses an important issue with private submodule access in Tekton pipelines and includes appropriate tests and validation steps. While the implementation is effective, there are opportunities to reduce code duplication, improve documentation, and enhance test coverage, particularly for edge cases. Additionally, separating unrelated changes (e.g., the sorting fix) into a different PR would improve focus and maintainability.

[wuhuizuo]

/approve

[ti-chi-bot]

[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

Details

Needs approval from an approver in each of these files:

• tekton/OWNERS [wuhuizuo]

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