Copilot commented on code in PR #475:
URL: https://github.com/apache/trafficserver-ci/pull/475#discussion_r4277212867
##########
jenkins/github/ubuntu.pipeline:
##########
@@ -16,30 +16,42 @@ pipeline {
steps {
dir('src') {
echo "${sha1}"
- checkout([$class: 'GitSCM',
- branches: [[name: sha1]],
- extensions: [
- // Keep this depth high enough for PreBuildMerge
to find the merge base
- // between the PR head and target branch. If it is
too low, checkout will
- // fail during the local merge with
shallow-history / missing-ancestor errors.
- [$class: 'CloneOption', honorRefspec: true,
shallow: true, depth: 1000, noTags: true, timeout: 20],
- // We have to set an identity for the merge step
because Git requires
- // the user.name and user.email to be set to do a
merge.
- [$class: "UserIdentity",
- name: "ATS CI User",
- email: "[email protected]"
- ],
- [$class: "PreBuildMerge",
- options: [
- mergeTarget: "${GITHUB_PR_TARGET_BRANCH}",
- fastForwardMode: "NO_FF",
- mergeRemote: "origin",
- mergeStrategy: "DEFAULT"
- ]
- ],
- ],
- userRemoteConfigs: [[url: github_url,
- refspec:
"+refs/heads/${GITHUB_PR_TARGET_BRANCH}:refs/remotes/origin/${GITHUB_PR_TARGET_BRANCH}
+refs/pull/${GITHUB_PR_NUMBER}/head:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/head
+refs/pull/${GITHUB_PR_NUMBER}/merge:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/merge"]]])
+ script {
+ int attempt = 0
+ retry(3) {
+ if (attempt > 0) {
+ // Spread retries out so jobs that failed
together do not come back together.
+ sleep(time: 30 * attempt + (int)
(Math.random() * 60), unit: 'SECONDS')
+ }
+ attempt++
+ timeout(time: 5, unit: 'MINUTES') {
+ checkout([$class: 'GitSCM',
Review Comment:
This retry/timeout/backoff block is duplicated across many pipeline files in
this PR. That makes future tweaks (e.g., retry count, timeout, or backoff
policy) easy to miss and costly to roll out. Consider extracting it into a
shared helper (e.g., a loaded Groovy script or a shared-library step, depending
on what this Jenkins setup supports) so the policy is defined in one place and
invoked from each pipeline.
##########
jenkins/github/ubuntu.pipeline:
##########
@@ -16,30 +16,42 @@ pipeline {
steps {
dir('src') {
echo "${sha1}"
- checkout([$class: 'GitSCM',
- branches: [[name: sha1]],
- extensions: [
- // Keep this depth high enough for PreBuildMerge
to find the merge base
- // between the PR head and target branch. If it is
too low, checkout will
- // fail during the local merge with
shallow-history / missing-ancestor errors.
- [$class: 'CloneOption', honorRefspec: true,
shallow: true, depth: 1000, noTags: true, timeout: 20],
- // We have to set an identity for the merge step
because Git requires
- // the user.name and user.email to be set to do a
merge.
- [$class: "UserIdentity",
- name: "ATS CI User",
- email: "[email protected]"
- ],
- [$class: "PreBuildMerge",
- options: [
- mergeTarget: "${GITHUB_PR_TARGET_BRANCH}",
- fastForwardMode: "NO_FF",
- mergeRemote: "origin",
- mergeStrategy: "DEFAULT"
- ]
- ],
- ],
- userRemoteConfigs: [[url: github_url,
- refspec:
"+refs/heads/${GITHUB_PR_TARGET_BRANCH}:refs/remotes/origin/${GITHUB_PR_TARGET_BRANCH}
+refs/pull/${GITHUB_PR_NUMBER}/head:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/head
+refs/pull/${GITHUB_PR_NUMBER}/merge:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/merge"]]])
+ script {
+ int attempt = 0
+ retry(3) {
+ if (attempt > 0) {
+ // Spread retries out so jobs that failed
together do not come back together.
+ sleep(time: 30 * attempt + (int)
(Math.random() * 60), unit: 'SECONDS')
Review Comment:
The PR description’s table states the jitter is '30 to 90 s' (try 2) and '60
to 120 s' (try 3), but `(int)(Math.random() * 60)` yields `0..59`, making the
actual ranges `30..89` and `60..119`. Either adjust the description/table or
change the random component to generate `0..60` (e.g., multiply by 61) so the
implementation matches the documented ranges.
##########
jenkins/github/ubuntu.pipeline:
##########
@@ -16,30 +16,42 @@ pipeline {
steps {
dir('src') {
echo "${sha1}"
- checkout([$class: 'GitSCM',
- branches: [[name: sha1]],
- extensions: [
- // Keep this depth high enough for PreBuildMerge
to find the merge base
- // between the PR head and target branch. If it is
too low, checkout will
- // fail during the local merge with
shallow-history / missing-ancestor errors.
- [$class: 'CloneOption', honorRefspec: true,
shallow: true, depth: 1000, noTags: true, timeout: 20],
- // We have to set an identity for the merge step
because Git requires
- // the user.name and user.email to be set to do a
merge.
- [$class: "UserIdentity",
- name: "ATS CI User",
- email: "[email protected]"
- ],
- [$class: "PreBuildMerge",
- options: [
- mergeTarget: "${GITHUB_PR_TARGET_BRANCH}",
- fastForwardMode: "NO_FF",
- mergeRemote: "origin",
- mergeStrategy: "DEFAULT"
- ]
- ],
- ],
- userRemoteConfigs: [[url: github_url,
- refspec:
"+refs/heads/${GITHUB_PR_TARGET_BRANCH}:refs/remotes/origin/${GITHUB_PR_TARGET_BRANCH}
+refs/pull/${GITHUB_PR_NUMBER}/head:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/head
+refs/pull/${GITHUB_PR_NUMBER}/merge:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/merge"]]])
+ script {
+ int attempt = 0
+ retry(3) {
+ if (attempt > 0) {
+ // Spread retries out so jobs that failed
together do not come back together.
+ sleep(time: 30 * attempt + (int)
(Math.random() * 60), unit: 'SECONDS')
+ }
+ attempt++
+ timeout(time: 5, unit: 'MINUTES') {
+ checkout([$class: 'GitSCM',
+ branches: [[name: sha1]],
+ extensions: [
+ // Keep this depth high enough for
PreBuildMerge to find the merge base
+ // between the PR head and target
branch. If it is too low, checkout will
+ // fail during the local merge with
shallow-history / missing-ancestor errors.
+ [$class: 'CloneOption', honorRefspec:
true, shallow: true, depth: 1000, noTags: true, timeout: 20],
+ // We have to set an identity for the
merge step because Git requires
+ // the user.name and user.email to be
set to do a merge.
+ [$class: "UserIdentity",
+ name: "ATS CI User",
+ email:
"[email protected]"
+ ],
+ [$class: "PreBuildMerge",
+ options: [
+ mergeTarget:
"${GITHUB_PR_TARGET_BRANCH}",
+ fastForwardMode: "NO_FF",
+ mergeRemote: "origin",
+ mergeStrategy: "DEFAULT"
+ ]
+ ],
+ ],
+ userRemoteConfigs: [[url: github_url,
+ refspec:
"+refs/heads/${GITHUB_PR_TARGET_BRANCH}:refs/remotes/origin/${GITHUB_PR_TARGET_BRANCH}
+refs/pull/${GITHUB_PR_NUMBER}/head:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/head
+refs/pull/${GITHUB_PR_NUMBER}/merge:refs/remotes/origin/pr/${GITHUB_PR_NUMBER}/merge"]]])
+ }
+ }
+ }
Review Comment:
When `timeout(...)` interrupts `checkout(...)`, the workspace can be left in
a partially-updated state (e.g., `.git/index.lock`, incomplete fetch/merge
state). The next `retry` attempt reuses the same `dir('src')` and can fail
immediately due to those leftovers, defeating the retry. Consider cleaning the
workspace on retries (e.g., only when `attempt > 0`: `deleteDir()` inside
`dir('src')` before `checkout`, or adding a GitSCM extension like
`WipeWorkspace` / `CleanBeforeCheckout` for retry attempts) so each attempt
starts from a known-good state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
