WIP: chore: add pac pipeline #312
Conversation
|
Warning Rate limit exceeded@l-qing has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 51 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughRemoved two Katanomi Build CRD manifests and added three Tekton PipelineRun manifests: doc and doc-pr PipelineRuns (with workspaces, params, timeouts, security/resource overrides) and a parameterized pr-manage PipelineRun scaffold for PR automation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant GitHost as "Git Host (PR / Comment)"
participant PaC as "Tekton PaC Controller"
participant Resolver as "Cluster Resolver"
participant PipelineRun as "PipelineRun (doc / doc-pr)"
participant PVC as "PVCs (source 25Gi, cache build-cache)"
User->>GitHost: Create PR / Comment / Push
GitHost->>PaC: Deliver event (on-comment / push)
PaC->>Resolver: Resolve pipelineRef (product-docs[-pr]-pipeline)
Resolver-->>PaC: Pipeline definition
PaC->>PVC: Provision/mount workspaces (source, cache)
PaC->>PipelineRun: Create PipelineRun (params, workspaces, timeouts, security/resource overrides)
PipelineRun->>PipelineRun: Execute tasks
PipelineRun->>GitHost: Report status (success / failure)
note right of PipelineRun `#F3F4F6`: Default TaskRun securityContext = root\ngit-clone overridden to non-root (65532)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.tekton/pr-manage.yaml (2)
6-6: External pipeline reference introduces coupling and availability risk.The pipeline is referenced from an external GitHub repository (
https://raw.githubusercontent.com/AlaudaDevops/toolbox/main/pr-cli/pipeline/pr-manage.yaml). This creates a dependency on that repository's availability and versioning. Consider:
- Documenting the fallback strategy if the external URL becomes unavailable
- Pinning to a specific Git commit/release rather than
mainfor reproducibility- Evaluating whether to vendor this pipeline into the repository
7-7: Document the comment-triggered command patterns.The regex pattern on line 7 is complex and lists many PR-management commands (help, rebase, lgtm, remove-lgtm, cherry-pick, assign, merge, ready, unassign, label, unlabel, check, retest, close, batch, checkbox, checkbox-issue). Consider adding a brief comment above the annotation documenting the supported commands for future maintainers.
.tekton/doc-pr-build.yaml (3)
23-23: Translate Chinese comment to English for consistency.Line 23 contains a Chinese comment: "下面这些变量都是 pac 触发时自动注入的" (roughly: "the following variables are auto-injected by PAC when triggered"). For consistency with the rest of the configuration, translate this to English.
- # 下面这些变量都是 pac 触发时自动注入的 + # The following variables are automatically injected by PAC on trigger
35-43: Document cluster resolver namespace requirement.The pipeline is resolved from the
idp-devnamespace via cluster resolver. Ensure:
- The
product-docs-pr-pipelinepipeline exists in theidp-devnamespace- The service account running this PipelineRun has permissions to reference pipelines in that namespace
- The
idp-devnamespace is stable and won't be removed or renamedConsider adding a comment documenting this cluster resolver dependency.
65-92: Review security context defaults (root by default).The default security context runs tasks as root (UID 0), with only
git-cloneoverridden to run as non-root (UID 65532). While this approach allows build tools to function, consider:
- Whether all downstream build tasks legitimately require root
- If a non-root default with explicit root overrides (instead of vice versa) would be more secure
- Documenting the security trade-offs for future maintainers
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.builds/doc-pr-build.yaml(0 hunks).tekton/doc-pr-build.yaml(1 hunks).tekton/pr-manage.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .builds/doc-pr-build.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pipelines as Code CI / doc-pr-build
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
.tekton/doc-pr-build.yaml (4)
45-58: Verify topolvm storage class availability in cluster.Line 49 specifies
storageClassName: topolvmfor the source workspace. Ensure:
- The
topolvmstorage class is deployed and available in the target cluster- It has sufficient capacity to handle multiple concurrent PipelineRuns (25Gi per run)
- This storage class is appropriate for ephemeral build artifacts
If this is cluster-specific, document the requirement or consider a dynamic default.
45-58: Clarify workspace secret mounting strategy.Lines 60-63 define a
basic-authworkspace mounting a secret named{{ git_auth_secret }}. The comment indicates PAC will replace this placeholder, but verify:
- The exact placeholder replacement behavior of your PAC controller version
- Whether the secret exists in the target namespace
- If this workspace is actually used by the referenced pipeline's git-clone task
- Consider whether the workspace naming and usage aligns with Tekton best practices (basic-auth vs git-credentials)
86-92: Validate compute resource sizing for documentation builds.Resources are set to 2 CPU/6Gi requests and 4 CPU/12Gi limits. Verify:
- These values are empirically appropriate for your documentation build pipeline (sphinx, webpack, etc.)
- They don't exceed available cluster capacity during concurrent builds
- Consider collecting actual usage metrics to right-size these values
9-14: CEL expression variables and syntax are correct.Verification confirms:
target_branchandeventare valid Pipelines as Code CEL variables.matches()is the correct method for regex patterns using RE2 syntax- The regex pattern
^(main|master|release-.*)$is syntactically valid and logically matches main, master, and release-* branchesConfirm that this pattern aligns with your actual branch naming conventions.
|
/retest |
|
/test doc-pr-build |
|
🔄 Retesting failed pipelines Triggered retests for: |
|
/retest |
|
/test doc-pr-build |
|
🔄 Retesting failed pipelines Triggered retests for: |
|
/ok-to-test |
l-qing
force-pushed
the
chore/add-pac-pipeline
branch
from
de80ac4 to
bcfb229
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.builds/doc-pr-build.yaml(0 hunks).tekton/doc-pr-build.yaml(1 hunks).tekton/pr-manage.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- .builds/doc-pr-build.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .tekton/pr-manage.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T08:42:48.099Z
Learnt from: l-qing
Repo: alauda/acp-docs PR: 312
File: .tekton/pr-manage.yaml:6-6
Timestamp: 2025-11-06T08:42:48.099Z
Learning: In Tekton Pipelines-as-Code (PAC), the `{{ trigger_comment }}` variable is a valid and documented variable that captures the full PR comment text that triggered the pipeline run. It is documented in the PAC GitOps Commands documentation and commonly used for parsing custom GitOps commands.
Applied to files:
.tekton/doc-pr-build.yaml
🔇 Additional comments (4)
.tekton/doc-pr-build.yaml (4)
21-24: Clarify the intent of the hardcoded doc-base parameter.The
doc-baseparameter is hardcoded tocontainer_platform. Confirm this is intentional and whether it should remain static or be parameterized per PR or branch (e.g., via PAC trigger comments).If this should be dynamic, consider exposing it as a parameter or parsing it from
{{ trigger_comment }}for flexibility.
37-45: Verify the referenced pipeline exists in the target cluster.This PipelineRun references
product-docs-pr-pipelinein theidp-devnamespace via the cluster resolver. If this pipeline does not exist or is not accessible from this cluster context, all triggered runs will fail.Please verify that the pipeline is deployed:
57-60: Verify that the build-cache PVC is provisioned.The cache workspace references a pre-existing PVC named
build-cache. If this PVC does not exist, pipeline runs will fail. Ensure it is created before merging, or document the prerequisite setup.Confirm the PVC exists:
10-15: Verify that target branch matching aligns with repository strategy.The CEL expression filters for branches matching
^(main|master|release-.*)$. Verify this covers all expected default/release branches in your repository. If the repository uses different naming conventions (e.g.,dev,develop,staging), the trigger will not fire on those branches.Confirm the branch naming strategy:
l-qing
force-pushed
the
chore/add-pac-pipeline
branch
3 times, most recently
from
5b31026 to
3ba35b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.tekton/doc-build.yaml (2)
67-82: Security context defaults to root; consider inverting to non-root by default.The
taskRunTemplatesecurityContext runs all tasks as root (UID 0) by default, creating a broad security surface. While thegit-clonetask overrides this with non-root (UID 65532), this pattern assumes most/all other tasks need root privileges.Recommendation: If most tasks in
product-docs-pipelinedo not require root, invert the default to run as non-root (e.g., UID 1000) and explicitly elevate only tasks that genuinely require root. If root is unavoidable for most tasks, add a clarifying comment documenting which build steps require elevation and why.This reflects the concern raised in a prior review on this file.
93-182: Significant code duplication in resource specifications across nine build tasks.Lines 93–182 define taskRunSpecs for nine build tasks, each with identical compute resource specifications (2 CPU/6Gi memory requests; 4 CPU/12Gi memory limits). This repetition violates DRY and increases maintenance burden.
Recommendation: If all nine tasks have the same resource requirements, extract them into a shared default or parameterize the values so they can be set once. If task-specific resource tuning is anticipated, document the intended differentiation or use a template mechanism to reduce duplication.
Example refactor direction (if using Kustomize or similar): Create a base configuration and use overlays or patches to apply the same resource spec to all nine tasks, or inject the resource spec as a parameter template.
.tekton/doc-pr-build.yaml (1)
67-75: Security context defaults to root; consider inverting to non-root by default.Same observation as
.tekton/doc-build.yaml: thetaskRunTemplateruns tasks as root (UID 0) by default, with onlygit-cloneoverridden to non-root (UID 65532). If most tasks inproduct-docs-pr-pipelinedo not require root privileges, invert the default to non-root and explicitly elevate only tasks that need elevation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.builds/doc-build.yaml(0 hunks).builds/doc-pr-build.yaml(0 hunks).tekton/doc-build.yaml(1 hunks).tekton/doc-pr-build.yaml(1 hunks).tekton/pr-manage.yaml(1 hunks)
💤 Files with no reviewable changes (2)
- .builds/doc-build.yaml
- .builds/doc-pr-build.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-06T08:42:48.119Z
Learnt from: l-qing
Repo: alauda/acp-docs PR: 312
File: .tekton/pr-manage.yaml:6-6
Timestamp: 2025-11-06T08:42:48.119Z
Learning: In Tekton Pipelines-as-Code (PAC), the `{{ trigger_comment }}` variable is a valid and documented variable that captures the full PR comment text that triggered the pipeline run. It is documented in the PAC GitOps Commands documentation and commonly used for parsing custom GitOps commands.
Applied to files:
.tekton/pr-manage.yaml.tekton/doc-pr-build.yaml.tekton/doc-build.yaml
🔇 Additional comments (4)
.tekton/pr-manage.yaml (2)
1-12: ✓ Valid PAC PipelineRun scaffold structure.The PipelineRun metadata, annotations, and parameter declarations are well-structured. The on-comment regex pattern comprehensively covers the intended GitOps commands, and all PAC interpolation variables (e.g.,
{{ trigger_comment }},{{ repo_owner }}, etc.) are correctly named per PAC documentation. As learnings confirm,{{ trigger_comment }}is a valid and documented PAC variable for capturing the PR comment that triggered the run.
25-72: Commented optional parameters document configuration surface clearly.The optional parameters (lines 25–72) are well-documented with inline comments explaining defaults and usage (image, lgtm_threshold, merge_method, etc.). Keeping them commented out preserves the scaffold's reusability—users can selectively enable only the overrides they need. This is a good design choice for a template.
.tekton/doc-build.yaml (1)
1-52: ✓ PipelineRun structure and resolver configuration are sound.The PipelineRun metadata, PAC annotations (on-comment, cancel-in-progress, on-cel-expression), timeouts, and parameters are all correctly configured. The cluster resolver approach is appropriate for referencing the external
product-docs-pipelinein theidp-devnamespace. Workspace setup (source, cache, basic-auth placeholder) is well-defined and ready for PAC controller substitution..tekton/doc-pr-build.yaml (1)
1-46: ✓ PipelineRun structure and PAC configuration are correct.The PipelineRun metadata, annotations (on-comment, on-cel-expression restricted to PR events), parameters, and cluster resolver configuration are all sound. The CEL expression correctly filters to PRs targeting main/master/release branches. Workspace and basic-auth placeholder setup aligns with best practices.
| - pipelineTaskName: build-docs | ||
| computeResources: | ||
| requests: | ||
| cpu: 2 | ||
| memory: 6Gi | ||
| limits: | ||
| cpu: 4 | ||
| memory: 12Gi |
There was a problem hiding this comment.
YAML structural error in computeResources for build-docs task.
The computeResources block at lines 87–94 is missing the required container name wrapper. Comparing to .tekton/doc-build.yaml (lines 93–102), the correct structure should be:
- pipelineTaskName: build-docs
computeResources:
- name: build # ← missing in current code
resources:
requests:
cpu: 2
memory: 6Gi
limits:
cpu: 4
memory: 12GiApply this diff to fix the structure:
- pipelineTaskName: build-docs
computeResources:
- requests:
+ - name: build
+ resources:
+ requests:
- cpu: 2
- memory: 6Gi
- limits:
- cpu: 4
- memory: 12Gi
+ cpu: 2
+ memory: 6Gi
+ limits:
+ cpu: 4
+ memory: 12Gi🤖 Prompt for AI Agents
In .tekton/doc-pr-build.yaml around lines 87 to 94, the computeResources block
for the build-docs pipelineTask is missing the required container entry; wrap
the resources under a named item by adding a "- name: build" (or the appropriate
container name) and move the requests/limits under "resources:" so the structure
becomes a list item with name and resources fields matching
.tekton/doc-build.yaml.
3ba35b9 to
009c505
Compare
|
/retest |
|
/test doc-build |
|
🔄 Retesting failed pipelines Triggered retests for: |
1 participant
Summary by CodeRabbit
New Features
Chores
Revert