internal/ci: address review feedback from CL 551352

Including some other bits. Drop the force-push of current tip of master to the trybot after the push of a Dispatch-Trailer commit for a trybot trigger. This step existed in a pre CL 551352 world in order that PR-based trybot triggers did not stall because of the commit in the PR being unrelated to the then tip of master in the trybot repo. In the new world, everything force-pushes to branches on the trybot repo (with retry) so there is no potential for conflict. This will also mean that we do a lot less unnecessary work on the trybot instance, and hence speed up trybot runs. Explicitly set an initial branch name wherever we do a git init in order that we don't have different behaviour depending on git config (some situations will default to master, others main). Use jq -r because we always want raw output. Define workflowFileExtension. We need to be able to rely on this value being constant in later CLs for use in workflow_dispatch. Drop direct use of the base package in ci_tool. We can leverage the fact that repo uses base as a template. Signed-off-by: Paul Jolly <paul@myitcv.io> Change-Id: I223d345a36d71de5039ed94b1d7a3b54400e00fc Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/552141 TryBot-Result: CUEcueckoo <cueckoo@cuelang.org> Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
cue-lang · Apr 4, 2023 · 06397b5 · 06397b5
1 parent 8d31dad
commit 06397b5
Show file tree

Hide file tree

Showing 10 changed files with 74 additions and 116 deletions.
diff --git a/.github/workflows/push_tip_to_trybot.yml b/.github/workflows/push_tip_to_trybot.yml
@@ -27,12 +27,13 @@ jobs:
         run: |-
           mkdir tmpgit
           cd tmpgit
-          git init
+          git init -b initialbranch
           git config user.name cueckoo
           git config user.email cueckoo@gmail.com
           git config http.https://github.com/.extraheader "AUTHORIZATION: basic $(echo -n cueckoo:${{ secrets.CUECKOO_GITHUB_PAT }} | base64)"
           git remote add origin https://review.gerrithub.io/a/cue-lang/cue
           git remote add trybot https://github.com/cue-lang/cue-trybot
+
           git fetch origin "${{ github.ref }}"
 
           success=false

diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -34,9 +34,14 @@ jobs:
           x="$(git log -1 --pretty='%(trailers:key=Dispatch-Trailer,valueonly)')"
           if [[ "$x" == "" ]]
           then
-              x=null
+             # Some steps rely on the presence or otherwise of the Dispatch-Trailer.
+             # We know that we don't have a Dispatch-Trailer in this situation,
+             # hence we use the JSON value null in order to represent that state.
+             # This means that GitHub expressions can determine whether a Dispatch-Trailer
+             # is present or not by checking whether the fromJSON() result of the
+             # output from this step is the JSON value null or not.
+             x=null
           fi
-          echo "x is $x"
           echo "value<<EOD" >> $GITHUB_OUTPUT
           echo "$x" >> $GITHUB_OUTPUT
           echo "EOD" >> $GITHUB_OUTPUT

diff --git a/.github/workflows/trybot.yml b/.github/workflows/trybot.yml
@@ -47,9 +47,14 @@ jobs:
           x="$(git log -1 --pretty='%(trailers:key=Dispatch-Trailer,valueonly)')"
           if [[ "$x" == "" ]]
           then
-              x=null
+             # Some steps rely on the presence or otherwise of the Dispatch-Trailer.
+             # We know that we don't have a Dispatch-Trailer in this situation,
+             # hence we use the JSON value null in order to represent that state.
+             # This means that GitHub expressions can determine whether a Dispatch-Trailer
+             # is present or not by checking whether the fromJSON() result of the
+             # output from this step is the JSON value null or not.
+             x=null
           fi
-          echo "x is $x"
           echo "value<<EOD" >> $GITHUB_OUTPUT
           echo "$x" >> $GITHUB_OUTPUT
           echo "EOD" >> $GITHUB_OUTPUT

diff --git a/.github/workflows/trybot_dispatch.yml b/.github/workflows/trybot_dispatch.yml
@@ -28,40 +28,27 @@ jobs:
         run: |-
           cat <<EOD >> $GITHUB_OUTPUT
           value<<DOE
-          {"type":"trybot","patchset":153,"CL":551352,"targetBranch":"master","ref":"refs/changes/52/551352/153"}
+          {"type":"trybot","CL":552141,"patchset":42,"targetBranch":"master","ref":"refs/changes/41/552141/42"}
           DOE
           EOD
       - if: github.event.client_payload.type != 'trybot'
         name: Trigger TryBot (fake data)
         run: |-
-          set -x
-
           mkdir tmpgit
           cd tmpgit
-          git init
+          git init -b initialbranch
           git config user.name cueckoo
           git config user.email cueckoo@gmail.com
           git config http.https://github.com/.extraheader "AUTHORIZATION: basic $(echo -n cueckoo:${{ secrets.CUECKOO_GITHUB_PAT }} | base64)"
           git remote add origin  https://review.gerrithub.io/a/cue-lang/cue
 
-          # We also (temporarily) get the default branch in order that
-          # we can "restore" the trybot repo to a good state for the
-          # current (i.e. previous) implementation of trybots which
-          # used PRs. If the target branch in the trybot repo is not
-          # current, then PR creation will fail because GitHub claims
-          # it cannot find any link between the commit in a PR (i.e.
-          # the CL under test in the previous setup) and the target
-          # branch which, under the new setup, might well currently
-          # be the commit from a CL.
-          git fetch origin ${{ fromJSON(steps.payload.outputs.value).targetBranch }}
-
           git fetch origin ${{ fromJSON(steps.payload.outputs.value).ref }}
-          git checkout -b ${{ fromJSON(steps.payload.outputs.value).targetBranch }} FETCH_HEAD
+          git checkout -b local_${{ fromJSON(steps.payload.outputs.value).targetBranch }} FETCH_HEAD
 
           # Error if we already have dispatchTrailer according to git log logic.
           # See earlier check for GitHub expression logic check.
           x="$(git log -1 --pretty='%(trailers:key=Dispatch-Trailer,valueonly)')"
-          if [ "$x" != "" ]
+          if [[ "$x" != "" ]]
           then
           	 echo "Ref ${{ fromJSON(steps.payload.outputs.value).ref }} already has a Dispatch-Trailer"
           	 exit 1
@@ -71,7 +58,10 @@ jobs:
           # substitute or quote capability. So we do that in shell. We also strip out the
           # indenting added by toJSON. We ensure that the type field is first in order
           # that we can safely check for specific types of dispatch trailer.
-          trailer="$(cat <<EOD | jq -c '{type} + .'
+          #
+          # Use bash heredoc so that JSON's use of double quotes does
+          # not get interpreted as shell.
+          trailer="$(cat <<EOD | jq -r -c '{type} + .'
           ${{ toJSON(fromJSON(steps.payload.outputs.value)) }}
           EOD
           )"
@@ -81,22 +71,7 @@ jobs:
           success=false
           for try in {1..20}; do
           	echo "Push to trybot try $try"
-          	if git push -f https://github.com/cue-lang/cue-trybot ${{ fromJSON(steps.payload.outputs.value).targetBranch }}:${{ fromJSON(steps.payload.outputs.value).targetBranch }}; then
-          		success=true
-          		break
-          	fi
-          	sleep 1
-          done
-          if ! $success; then
-          	echo "Giving up"
-          	exit 1
-          fi
-
-          # Restore the default branch on the trybot repo to be the tip of the main repo
-          success=false
-          for try in {1..20}; do
-          	echo "Push to trybot try $try"
-          	if git push -f https://github.com/cue-lang/cue-trybot origin/${{ fromJSON(steps.payload.outputs.value).targetBranch }}:${{ fromJSON(steps.payload.outputs.value).targetBranch }}; then
+          	if git push -f https://github.com/cue-lang/cue-trybot local_${{ fromJSON(steps.payload.outputs.value).targetBranch }}:${{ fromJSON(steps.payload.outputs.value).targetBranch }}; then
           		success=true
           		break
           	fi
@@ -109,34 +84,21 @@ jobs:
       - if: github.event.client_payload.type == 'trybot'
         name: Trigger TryBot (repository_dispatch payload)
         run: |-
-          set -x
-
           mkdir tmpgit
           cd tmpgit
-          git init
+          git init -b initialbranch
           git config user.name cueckoo
           git config user.email cueckoo@gmail.com
           git config http.https://github.com/.extraheader "AUTHORIZATION: basic $(echo -n cueckoo:${{ secrets.CUECKOO_GITHUB_PAT }} | base64)"
           git remote add origin  https://review.gerrithub.io/a/cue-lang/cue
 
-          # We also (temporarily) get the default branch in order that
-          # we can "restore" the trybot repo to a good state for the
-          # current (i.e. previous) implementation of trybots which
-          # used PRs. If the target branch in the trybot repo is not
-          # current, then PR creation will fail because GitHub claims
-          # it cannot find any link between the commit in a PR (i.e.
-          # the CL under test in the previous setup) and the target
-          # branch which, under the new setup, might well currently
-          # be the commit from a CL.
-          git fetch origin ${{ github.event.client_payload.targetBranch }}
-
           git fetch origin ${{ github.event.client_payload.ref }}
-          git checkout -b ${{ github.event.client_payload.targetBranch }} FETCH_HEAD
+          git checkout -b local_${{ github.event.client_payload.targetBranch }} FETCH_HEAD
 
           # Error if we already have dispatchTrailer according to git log logic.
           # See earlier check for GitHub expression logic check.
           x="$(git log -1 --pretty='%(trailers:key=Dispatch-Trailer,valueonly)')"
-          if [ "$x" != "" ]
+          if [[ "$x" != "" ]]
           then
           	 echo "Ref ${{ github.event.client_payload.ref }} already has a Dispatch-Trailer"
           	 exit 1
@@ -146,7 +108,10 @@ jobs:
           # substitute or quote capability. So we do that in shell. We also strip out the
           # indenting added by toJSON. We ensure that the type field is first in order
           # that we can safely check for specific types of dispatch trailer.
-          trailer="$(cat <<EOD | jq -c '{type} + .'
+          #
+          # Use bash heredoc so that JSON's use of double quotes does
+          # not get interpreted as shell.
+          trailer="$(cat <<EOD | jq -r -c '{type} + .'
           ${{ toJSON(github.event.client_payload) }}
           EOD
           )"
@@ -156,22 +121,7 @@ jobs:
           success=false
           for try in {1..20}; do
           	echo "Push to trybot try $try"
-          	if git push -f https://github.com/cue-lang/cue-trybot ${{ github.event.client_payload.targetBranch }}:${{ github.event.client_payload.targetBranch }}; then
-          		success=true
-          		break
-          	fi
-          	sleep 1
-          done
-          if ! $success; then
-          	echo "Giving up"
-          	exit 1
-          fi
-
-          # Restore the default branch on the trybot repo to be the tip of the main repo
-          success=false
-          for try in {1..20}; do
-          	echo "Push to trybot try $try"
-          	if git push -f https://github.com/cue-lang/cue-trybot origin/${{ github.event.client_payload.targetBranch }}:${{ github.event.client_payload.targetBranch }}; then
+          	if git push -f https://github.com/cue-lang/cue-trybot local_${{ github.event.client_payload.targetBranch }}:${{ github.event.client_payload.targetBranch }}; then
           		success=true
           		break
           	fi

diff --git a/internal/ci/base/base.cue b/internal/ci/base/base.cue
@@ -55,6 +55,8 @@ botGerritHubUser:                   *botGitHubUser | string
 botGerritHubUserPasswordSecretsKey: *(strings.ToUpper(botGitHubUser) + "_GERRITHUB_PASSWORD") | string
 botGerritHubUserEmail:              *botGitHubUserEmail | string
 
+workflowFileExtension: ".yml"
+
 linuxMachine: string
 
 codeReview: #codeReview & {

diff --git a/internal/ci/base/gerrithub.cue b/internal/ci/base/gerrithub.cue
@@ -67,7 +67,12 @@ trybotDispatchWorkflow: bashWorkflow & {
 					name: "Write fake payload"
 					id:   "payload"
 					if:   "github.repository == '\(githubRepositoryPath)' && github.ref == 'refs/heads/\(testDefaultBranch)'"
-					run:  #"""
+
+					// Use bash heredocs so that JSON's use of double quotes does
+					// not get interpreted as shell.  Both in the running of the
+					// command itself, which itself is the echo-ing of a command to
+					// $GITHUB_OUTPUT.
+					run: #"""
 						cat <<EOD >> $GITHUB_OUTPUT
 						value<<DOE
 						\#(encjson.Marshal(#dummyDispatch))
@@ -85,38 +90,27 @@ trybotDispatchWorkflow: bashWorkflow & {
 				// repository_dispatch payload is set, and one if not (i.e. we use
 				// the fake payload).
 				for v in cases {
+					let localBranchExpr = "local_${{ \(v.expr).targetBranch }}"
+					let targetBranchExpr = "${{ \(v.expr).targetBranch }}"
 					json.#step & {
 						name: "Trigger \(trybot.name) (\(v.nameSuffix))"
 						if:   "github.event.client_payload.type \(v.condition) '\(trybot.key)'"
 						run:  """
-						set -x
-
 						mkdir tmpgit
 						cd tmpgit
-						git init
+						git init -b initialbranch
 						git config user.name \(botGitHubUser)
 						git config user.email \(botGitHubUserEmail)
 						git config http.https://github.com/.extraheader "AUTHORIZATION: basic $(echo -n \(botGitHubUser):${{ secrets.\(botGitHubUserTokenSecretsKey) }} | base64)"
 						git remote add origin  \(gerritHubRepositoryURL)
 
-						# We also (temporarily) get the default branch in order that
-						# we can "restore" the trybot repo to a good state for the
-						# current (i.e. previous) implementation of trybots which
-						# used PRs. If the target branch in the trybot repo is not
-						# current, then PR creation will fail because GitHub claims
-						# it cannot find any link between the commit in a PR (i.e.
-						# the CL under test in the previous setup) and the target
-						# branch which, under the new setup, might well currently
-						# be the commit from a CL.
-						git fetch origin ${{ \(v.expr).targetBranch }}
-
 						git fetch origin ${{ \(v.expr).ref }}
-						git checkout -b ${{ \(v.expr).targetBranch }} FETCH_HEAD
+						git checkout -b \(localBranchExpr) FETCH_HEAD
 
 						# Error if we already have dispatchTrailer according to git log logic.
 						# See earlier check for GitHub expression logic check.
 						x="$(git log -1 --pretty='%(trailers:key=\(dispatchTrailer),valueonly)')"
-						if [ "$x" != "" ]
+						if [[ "$x" != "" ]]
 						then
 							 echo "Ref ${{ \(v.expr).ref }} already has a \(dispatchTrailer)"
 							 exit 1
@@ -126,7 +120,10 @@ trybotDispatchWorkflow: bashWorkflow & {
 						# substitute or quote capability. So we do that in shell. We also strip out the
 						# indenting added by toJSON. We ensure that the type field is first in order
 						# that we can safely check for specific types of dispatch trailer.
-						trailer="$(cat <<EOD | jq -c '{type} + .'
+						#
+						# Use bash heredoc so that JSON's use of double quotes does
+						# not get interpreted as shell.
+						trailer="$(cat <<EOD | jq -r -c '{type} + .'
 						${{ toJSON(\(v.expr)) }}
 						EOD
 						)"
@@ -136,22 +133,7 @@ trybotDispatchWorkflow: bashWorkflow & {
 						success=false
 						for try in {1..20}; do
 							echo "Push to trybot try $try"
-							if git push -f \(trybotRepositoryURL) ${{ \(v.expr).targetBranch }}:${{ \(v.expr).targetBranch }}; then
-								success=true
-								break
-							fi
-							sleep 1
-						done
-						if ! $success; then
-							echo "Giving up"
-							exit 1
-						fi
-
-						# Restore the default branch on the trybot repo to be the tip of the main repo
-						success=false
-						for try in {1..20}; do
-							echo "Push to trybot try $try"
-							if git push -f \(trybotRepositoryURL) origin/${{ \(v.expr).targetBranch }}:${{ \(v.expr).targetBranch }}; then
+							if git push -f \(trybotRepositoryURL) \(localBranchExpr):\(targetBranchExpr); then
 								success=true
 								break
 							fi
@@ -192,12 +174,13 @@ pushTipToTrybotWorkflow: bashWorkflow & {
 				run:  """
 						mkdir tmpgit
 						cd tmpgit
-						git init
+						git init -b initialbranch
 						git config user.name \(botGitHubUser)
 						git config user.email \(botGitHubUserEmail)
 						git config http.https://github.com/.extraheader "AUTHORIZATION: basic $(echo -n \(botGitHubUser):${{ secrets.\(botGitHubUserTokenSecretsKey) }} | base64)"
 						git remote add origin \(gerritHubRepositoryURL)
 						git remote add trybot \(trybotRepositoryURL)
+
 						git fetch origin "${{ github.ref }}"
 
 						success=false

diff --git a/internal/ci/base/github.cue b/internal/ci/base/github.cue
@@ -70,9 +70,14 @@ checkoutCode: {
 					x="$(git log -1 --pretty='%(trailers:key=\(dispatchTrailer),valueonly)')"
 					if [[ "$x" == "" ]]
 					then
-					    x=null
+					   # Some steps rely on the presence or otherwise of the Dispatch-Trailer.
+					   # We know that we don't have a Dispatch-Trailer in this situation,
+					   # hence we use the JSON value null in order to represent that state.
+					   # This means that GitHub expressions can determine whether a Dispatch-Trailer
+					   # is present or not by checking whether the fromJSON() result of the
+					   # output from this step is the JSON value null or not.
+					   x=null
 					fi
-					echo "x is $x"
 					echo "value<<EOD" >> $GITHUB_OUTPUT
 					echo "$x" >> $GITHUB_OUTPUT
 					echo "EOD" >> $GITHUB_OUTPUT
@@ -314,6 +319,12 @@ dispatchTrailer: "Dispatch-Trailer"
 // approximation of the logic employed by git log.
 containsDispatchTrailer: {
 	#type?: string
+
+	// If we have a value for #type, then match against that value.
+	// Otherwise the best we can do is match against:
+	//
+	//     Dispatch-Trailer: {"type:}
+	//
 	let _typeCheck = [ if #type != _|_ {#type + "\""}, ""][0]
 	"""
 	(contains(\(_dispatchTrailerVariable), '\n\(dispatchTrailer): {"type":"\(_typeCheck)'))

diff --git a/internal/ci/ci_tool.cue b/internal/ci/ci_tool.cue
@@ -19,7 +19,6 @@ import (
 	"encoding/yaml"
 	"tool/file"
 
-	"cuelang.org/go/internal/ci/base"
 	"cuelang.org/go/internal/ci/repo"
 	"cuelang.org/go/internal/ci/github"
 )
@@ -54,11 +53,11 @@ command: gen: {
 			}
 		}
 		for _workflowName, _workflow in github.workflows {
-			let _filename = _workflowName + ".yml"
+			let _filename = _workflowName + repo.workflowFileExtension
 			"generate \(_filename)": file.Create & {
 				$after: [ for v in remove {v}]
 				filename: path.Join([_dir, _filename], _goos)
-				let donotedit = base.doNotEditMessage & {#generatedBy: "internal/ci/ci_tool.cue", _}
+				let donotedit = repo.doNotEditMessage & {#generatedBy: "internal/ci/ci_tool.cue", _}
 				contents: "# \(donotedit)\n\n\(yaml.Marshal(_workflow))"
 			}
 		}
@@ -68,7 +67,7 @@ command: gen: {
 command: gen: codereviewcfg: file.Create & {
 	_dir:     path.FromSlash("../../", path.Unix)
 	filename: path.Join([_dir, "codereview.cfg"], _goos)
-	let res = base.toCodeReviewCfg & {#input: repo.codeReview, _}
-	let donotedit = base.doNotEditMessage & {#generatedBy: "internal/ci/ci_tool.cue", _}
+	let res = repo.toCodeReviewCfg & {#input: repo.codeReview, _}
+	let donotedit = repo.doNotEditMessage & {#generatedBy: "internal/ci/ci_tool.cue", _}
 	contents: "# \(donotedit)\n\n\(res)\n"
 }
diff --git a/internal/ci/github/gen_trybot_dispatch.cue b/internal/ci/github/gen_trybot_dispatch.cue
@@ -1,3 +1,6 @@
 package github
 
-dummyDispatch: patchset: 153
+dummyDispatch: {
+    CL:       552141
+    patchset: 42
+}
diff --git a/internal/ci/github/workflows.cue b/internal/ci/github/workflows.cue
@@ -48,5 +48,4 @@ workflows: close({
 
 dummyDispatch: _repo.#dispatch & {
 	type: _repo.trybot.key
-	CL:   551352
 }