Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support an Impacts All PATH Filter #39

Merged
merged 25 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions .github/workflows/compute_impacted_tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v3
with:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone all. git cloning has been moved to the prerequisites step.

fetch-depth: 0

- name: Setup Bazel
# trunk-ignore(semgrep): Trust third-party `bazelbuild` GH Action
Expand All @@ -20,8 +22,10 @@ jobs:
run: ./src/scripts/compute_impacted_targets.sh
shell: bash
env:
MERGE_INSTANCE_BRANCH: main
MERGE_INSTANCE_BRANCH: do_not_delete/stable_test_branch
MERGE_INSTANCE_BRANCH_HEAD_SHA: 097c8259c2e18da92f6189849ebc0f7f6dc624e5
PR_BRANCH: do_not_delete/stable_test_branch
PR_BRANCH_HEAD_SHA: 097c8259c2e18da92f6189849ebc0f7f6dc624e5
VERBOSE: 1
WORKSPACE_PATH: ./tests/simple_bazel_workspace
BAZEL_STARTUP_OPTIONS: --host_jvm_args=-Xmx12G,--block_for_lock,--client_debug
Expand All @@ -31,6 +35,6 @@ jobs:
shell: bash
run: |
target_count=`cat ${{ steps.compute.outputs.impacted_targets_out }} | wc -l`
if [[ $target_count -ne 2 ]]; then
if [[ $target_count -ne 0 ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We moved sha computation outside of the compute_impacted_targets script, and expect it as an input into the script. As opposed to running tests with a stable test branch and a moving main, we now run tests with a stable test branch and a stable main, such that we can pin the shas. In that case, we would expect no impacted targets to be computed.

exit 1
fi
30 changes: 27 additions & 3 deletions action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -29,36 +29,59 @@ inputs:
description: A path to the Bazel executable. Defaults to PATH.
required: false
default: bazel
impact-all-filters-path:
description:
A path to a list of filters to identify whether `ALL` impacted targets should be considered.
See https://github.com/dorny/paths-filter/blob/master/.github/filters.yml for an example.
required: false
default: ""

runs:
using: composite
steps:
- name: Detect changed paths
id: detect-changed-paths
if: ${{ inputs.impact-all-filters-path != '' }}
# trunk-ignore(semgrep/yaml.github-actions.security.third-party-action-not-pinned-to-commit-sha.third-party-action-not-pinned-to-commit-sha)
uses: dorny/paths-filter@v2
with:
filters: ${{ inputs.impact-all-filters-path }}

- name: Prerequisites
id: prerequisites
run: ${GITHUB_ACTION_PATH}/src/scripts/prerequisites.sh
shell: bash
env:
DEFAULT_BRANCH: ${{ github.event.repository.default_branch }}
TARGET_BRANCH: ${{ inputs.target-branch }}
PR_BRANCH: ${{ github.head_ref }}
WORKSPACE_PATH: ${{ inputs.bazel-workspace-path }}
BAZEL_PATH: ${{ inputs.bazel-path }}
IMPACTS_FILTERS_CHANGES: ${{ steps.detect-changed-paths.outputs.changes }}

- name: Install Bazel in PATH
if: ${{ steps.prerequisites.outputs.requires_default_bazel_installation == 'true' }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like everything with if: ${{steps.prerequisites.outputs.impacts_all_detected == 'false' }} should be put into its own workflow and only run on that if, so that:

  1. Clear separation of logic
  2. You don't have to repeat that if everywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a few changes to cleanup the workflow:
-- always install jq, regardless of impacts_all_detected
-- set requires_default_bazel_installation to false if impacts_all_detected==true.

I think this cleans up the workflow, PTAL.

if:
${{ steps.prerequisites.outputs.requires_default_bazel_installation == 'true' }} && ${{
steps.prerequisites.outputs.impacts_all_detected == 'false' }}
# trunk-ignore(semgrep): Trust third-party `bazelbuild` GH Action
uses: bazelbuild/setup-bazelisk@v2

- name: Setup jq
if: ${{ steps.prerequisites.outputs.impacts_all_detected == 'false' }}
# trunk-ignore(semgrep): Trust third-party action to install JQ. Source code: https://github.com/dcarbone/install-jq-action/
uses: dcarbone/install-jq-action@v1.0.1

- name: Compute Impacted Targets
id: compute-impacted-targets
run: ${GITHUB_ACTION_PATH}/src/scripts/compute_impacted_targets.sh
if: ${{ steps.prerequisites.outputs.impacts_all_detected == 'false' }}
shell: bash
env:
MERGE_INSTANCE_BRANCH: ${{ steps.prerequisites.outputs.merge_instance_branch }}
PR_BRANCH: ${{ github.head_ref }}
MERGE_INSTANCE_BRANCH_HEAD_SHA:
${{ steps.prerequisites.outputs.merge_instance_branch_head_sha }}
PR_BRANCH: ${{ steps.prerequisites.outputs.pr_branch }}
PR_BRANCH_HEAD_SHA: ${{ steps.prerequisites.outputs.pr_branch_head_sha }}
VERBOSE: ${{ inputs.verbose }}
WORKSPACE_PATH: ${{ steps.prerequisites.outputs.workspace_path }}
BAZEL_PATH: ${{ inputs.bazel-path }}
Expand All @@ -73,5 +96,6 @@ runs:
REPOSITORY: ${{ github.repository }}
TARGET_BRANCH: ${{ steps.prerequisites.outputs.merge_instance_branch }}
PR_NUMBER: ${{ github.event.pull_request.number }}
PR_SHA: ${{ steps.compute-impacted-targets.outputs.git_commit }}
PR_SHA: ${{ steps.prerequisites.outputs.pr_branch_head_sha }}
IMPACTED_TARGETS_FILE: ${{ steps.compute-impacted-targets.outputs.impacted_targets_out }}
IMPACTS_ALL_DETECTED: ${{ steps.prerequisites.outputs.impacts_all_detected }}
31 changes: 12 additions & 19 deletions src/scripts/compute_impacted_targets.sh
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@ if [[ (-z ${MERGE_INSTANCE_BRANCH}) || (-z ${PR_BRANCH}) ]]; then
exit 2
fi

if [[ (-z ${MERGE_INSTANCE_BRANCH_HEAD_SHA}) || (-z ${PR_BRANCH_HEAD_SHA}) ]]; then
echo "Missing sha"
exit 2
fi

if [[ -z ${WORKSPACE_PATH} ]]; then
echo "Missing workspace path"
exit 2
Expand Down Expand Up @@ -56,32 +61,21 @@ fetchRemoteGitHistory() {
logIfVerbose "...done!"
}

fetchRemoteGitHistory "${MERGE_INSTANCE_BRANCH}"
fetchRemoteGitHistory "${PR_BRANCH}"

git switch "${MERGE_INSTANCE_BRANCH}"
merge_instance_branch_head_sha=$(git rev-parse "${MERGE_INSTANCE_BRANCH}")
ifVerbose echo "Merge Instance Branch Head= ${merge_instance_branch_head_sha}"

git switch "${PR_BRANCH}"
pr_branch_head_sha=$(git rev-parse "${PR_BRANCH}")
ifVerbose echo "PR Branch Head= ${pr_branch_head_sha}"

## Verbose logging for the Merge Instance and PR branch.
if [[ -n ${VERBOSE} ]]; then
# Find the merge base of the two branches
merge_base_sha=$(git merge-base "${merge_instance_branch_head_sha}" "${pr_branch_head_sha}")
merge_base_sha=$(git merge-base "${MERGE_INSTANCE_BRANCH_HEAD_SHA}" "${PR_BRANCH_HEAD_SHA}")
echo "Merge Base= ${merge_base_sha}"

# Find the number of commits between the merge base and the merge instance's HEAD
merge_instance_depth=$(git rev-list "${merge_base_sha}".."${merge_instance_branch_head_sha}" | wc -l)
merge_instance_depth=$(git rev-list "${merge_base_sha}".."${MERGE_INSTANCE_BRANCH_HEAD_SHA}" | wc -l)
echo "Merge Instance Depth= ${merge_instance_depth}"

git switch "${MERGE_INSTANCE_BRANCH}"
git log -n "${merge_instance_depth}" --oneline

# Find the number of commits between the merge base and the PR's HEAD
pr_depth=$(git rev-list "${merge_base_sha}".."${pr_branch_head_sha}" | wc -l)
pr_depth=$(git rev-list "${merge_base_sha}".."${PR_BRANCH_HEAD_SHA}" | wc -l)
echo "PR Depth= ${pr_depth}"

git switch "${PR_BRANCH}"
Expand All @@ -94,9 +88,9 @@ _java -jar bazel-diff.jar -V
_bazel version # Does not require running with startup options.

# Output Files
merge_instance_branch_out=./${merge_instance_branch_head_sha}
merge_instance_with_pr_branch_out=./${pr_branch_head_sha}_${merge_instance_branch_head_sha}
impacted_targets_out=./impacted_targets_${pr_branch_head_sha}
merge_instance_branch_out=./${MERGE_INSTANCE_BRANCH_HEAD_SHA}
merge_instance_with_pr_branch_out=./${PR_BRANCH_HEAD_SHA}_${MERGE_INSTANCE_BRANCH_HEAD_SHA}
impacted_targets_out=./impacted_targets_${PR_BRANCH_HEAD_SHA}

# Generate Hashes for the Merge Instance Branch
git switch "${MERGE_INSTANCE_BRANCH}"
Expand All @@ -110,8 +104,7 @@ bazelDiff generate-hashes --bazelPath="${BAZEL_PATH}" --workspacePath="${WORKSPA
bazelDiff get-impacted-targets --startingHashes="${merge_instance_branch_out}" --finalHashes="${merge_instance_with_pr_branch_out}" --output="${impacted_targets_out}"

num_impacted_targets=$(wc -l <"${impacted_targets_out}")
echo "Computed ${num_impacted_targets} targets for sha ${pr_branch_head_sha}"
echo "Computed ${num_impacted_targets} targets for sha ${PR_BRANCH_HEAD_SHA}"

# Outputs
echo "git_commit=${pr_branch_head_sha}" >>"${GITHUB_OUTPUT}"
echo "impacted_targets_out=${impacted_targets_out}" >>"${GITHUB_OUTPUT}"
33 changes: 33 additions & 0 deletions src/scripts/prerequisites.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,15 @@

set -euo pipefail

# NOTE: We cannot assume that the checked out Git repo (e.g. via actions-checkout)
# was a shallow vs a complete clone. The `--depth` options deepens the commit history
# in both clone modes: https://git-scm.com/docs/fetch-options#Documentation/fetch-options.txt---depthltdepthgt
fetchRemoteGitHistory() {
git fetch --quiet --depth=2147483647 origin "$@"
}

# trunk-ignore(shellcheck)
pr_branch="${PR_BRANCH}"
merge_instance_branch="${TARGET_BRANCH}"
if [[ -z ${merge_instance_branch} ]]; then
merge_instance_branch="${DEFAULT_BRANCH}"
Expand All @@ -25,8 +34,32 @@ if [[ ${BAZEL_PATH} == "bazel" ]]; then
fi
fi

changes_count=0
impacts_all_detected="false"
if [[ -n ${IMPACTS_FILTERS_CHANGES+x} ]]; then
changes_count=$(echo "${IMPACTS_FILTERS_CHANGES}" | jq length)
if [[ ${changes_count} -gt 0 ]]; then
impacts_all_detected="true"
fi
fi

fetchRemoteGitHistory "${merge_instance_branch}"
fetchRemoteGitHistory "${pr_branch}"

git switch "${merge_instance_branch}"
merge_instance_branch_head_sha=$(git rev-parse "${merge_instance_branch}")

git switch "${pr_branch}"
pr_branch_head_sha=$(git rev-parse "${pr_branch}")

echo "Identified changes: " "${impacts_all_detected}"

# Outputs
# trunk-ignore(shellcheck/SC2129)
echo "merge_instance_branch=${merge_instance_branch}" >>"${GITHUB_OUTPUT}"
echo "merge_instance_branch_head_sha=${merge_instance_branch_head_sha}" >>"${GITHUB_OUTPUT}"
echo "pr_branch=${pr_branch}" >>"${GITHUB_OUTPUT}"
echo "pr_branch_head_sha=${pr_branch_head_sha}" >>"${GITHUB_OUTPUT}"
echo "impacts_all_detected=${impacts_all_detected}" >>"${GITHUB_OUTPUT}"
echo "workspace_path=${workspace_path}" >>"${GITHUB_OUTPUT}"
echo "requires_default_bazel_installation=${requires_default_bazel_installation}" >>"${GITHUB_OUTPUT}"
57 changes: 33 additions & 24 deletions src/scripts/upload_impacted_targets.sh
Original file line number Diff line number Diff line change
Expand Up @@ -22,27 +22,11 @@ if [[ (-z ${PR_NUMBER}) || (-z ${PR_SHA}) ]]; then
exit 2
fi

if [[ -z ${IMPACTED_TARGETS_FILE+x} ]]; then
echo "Missing Impacted Targets File"
exit 2
fi

# API URL
if [[ -z ${API_URL+x} ]]; then
API_URL="https://api.trunk.io:443/v1/setImpactedTargets"
fi

# Reformat the impacted targets into JSON array and pipe into a new file.
IMPACTED_TARGETS_JSON_TMP="./impacted_targets_json_tmp"
touch "${IMPACTED_TARGETS_JSON_TMP}"
mapfile -t impacted_targets_array <"${IMPACTED_TARGETS_FILE}"
IMPACTED_TARGETS=$(printf '%s\n' "${impacted_targets_array[@]}" | jq -R . | jq -s .)
if [[ -z ${IMPACTED_TARGETS} ]]; then
echo "[]" >"${IMPACTED_TARGETS_JSON_TMP}"
else
echo "${IMPACTED_TARGETS}" >"${IMPACTED_TARGETS_JSON_TMP}"
fi

REPO_BODY=$(
jq --null-input \
--arg host "github.com" \
Expand All @@ -58,14 +42,40 @@ PR_BODY=$(
'{ "number": $number, "sha": $sha }'
)

num_impacted_targets=""
POST_BODY="./post_body_tmp"
jq --null-input \
--argjson repo "${REPO_BODY}" \
--argjson pr "${PR_BODY}" \
--slurpfile impactedTargets "${IMPACTED_TARGETS_JSON_TMP}" \
--arg targetBranch "${TARGET_BRANCH}" \
'{ "repo": $repo, "pr": $pr, "targetBranch": $targetBranch, "impactedTargets": $impactedTargets | .[0] | map(select(length > 0)) }' \
>"${POST_BODY}"
if [[ ${IMPACTS_ALL_DETECTED} == 'true' ]]; then
jq --null-input \
--argjson repo "${REPO_BODY}" \
--argjson pr "${PR_BODY}" \
--arg impactedTargets "ALL" \
--arg targetBranch "${TARGET_BRANCH}" \
'{ "repo": $repo, "pr": $pr, "targetBranch": $targetBranch, "impactedTargets": $impactedTargets }' \
>"${POST_BODY}"

num_impacted_targets="'ALL'"
else
# Reformat the impacted targets into JSON array and pipe into a new file.
IMPACTED_TARGETS_JSON_TMP="./impacted_targets_json_tmp"
touch "${IMPACTED_TARGETS_JSON_TMP}"
mapfile -t impacted_targets_array <"${IMPACTED_TARGETS_FILE}"
IMPACTED_TARGETS=$(printf '%s\n' "${impacted_targets_array[@]}" | jq -R . | jq -s .)
if [[ -z ${IMPACTED_TARGETS} ]]; then
echo "[]" >"${IMPACTED_TARGETS_JSON_TMP}"
else
echo "${IMPACTED_TARGETS}" >"${IMPACTED_TARGETS_JSON_TMP}"
fi

jq --null-input \
--argjson repo "${REPO_BODY}" \
--argjson pr "${PR_BODY}" \
--slurpfile impactedTargets "${IMPACTED_TARGETS_JSON_TMP}" \
--arg targetBranch "${TARGET_BRANCH}" \
'{ "repo": $repo, "pr": $pr, "targetBranch": $targetBranch, "impactedTargets": $impactedTargets | .[0] | map(select(length > 0)) }' \
>"${POST_BODY}"

num_impacted_targets=$(wc -l <"${IMPACTED_TARGETS_FILE}")
fi

HTTP_STATUS_CODE=$(
curl -s -o /dev/null -w '%{http_code}' -X POST \
Expand All @@ -77,7 +87,6 @@ HTTP_STATUS_CODE=$(
EXIT_CODE=0
COMMENT_TEXT=""
if [[ ${HTTP_STATUS_CODE} == 200 ]]; then
num_impacted_targets=$(wc -l <"${IMPACTED_TARGETS_FILE}")
COMMENT_TEXT="✨ Uploaded ${num_impacted_targets} impacted targets for ${PR_NUMBER} @ ${PR_SHA}"
else
EXIT_CODE=1
Expand Down
19 changes: 15 additions & 4 deletions tests/upload.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import util from "node:util";

const PORT = 4567;

type ImpactedTargets = string[] | "ALL";

const fetchUrl = (path: string) => `http://localhost:${PORT}${path}`;
const UPLOAD_IMPACTED_TARGETS_SCRIPT = "src/scripts/upload_impacted_targets.sh";
const ENV_VARIABLES: Record<string, string> = {
Expand All @@ -18,6 +20,7 @@ const ENV_VARIABLES: Record<string, string> = {
PR_NUMBER: "123",
PR_SHA: "test-pr-sha",
IMPACTED_TARGETS_FILE: "/tmp/test-impacted-targets-file",
IMPACTS_ALL_DETECTED: "false",
API_URL: fetchUrl("/testUploadImpactedTargets"),
};
const exportEnv = (env: Record<string, string>) =>
Expand All @@ -32,12 +35,14 @@ let server: http.Server;
let uploadedImpactedTargetsPayload = [null, null];

const runUploadTargets = async (
impactedTargets: string[],
impactedTargets: ImpactedTargets,
env: Record<string, string> = ENV_VARIABLES,
) => {
// The bazel / glob / ... scripts are responsible for populating these files.
// Verify that the upload works as intended.
fs.writeFileSync(env.IMPACTED_TARGETS_FILE, impactedTargets.join("\n"));
if (impactedTargets !== "ALL") {
fs.writeFileSync(env.IMPACTED_TARGETS_FILE, impactedTargets.join("\n"));
}

const runScript = util.promisify(exec.exec)(
`${exportEnv(env)} ${UPLOAD_IMPACTED_TARGETS_SCRIPT}`,
Expand All @@ -46,7 +51,7 @@ const runUploadTargets = async (
await runScript;
};

const expectImpactedTargetsUpload = (impactedTargets: string[]): void => {
const expectImpactedTargetsUpload = (impactedTargets: ImpactedTargets): void => {
const { API_TOKEN, REPOSITORY, TARGET_BRANCH, PR_NUMBER, PR_SHA } = ENV_VARIABLES;
const [actualToken, actualBody] = uploadedImpactedTargetsPayload;
expect(actualToken).toEqual(API_TOKEN);
Expand Down Expand Up @@ -121,11 +126,17 @@ test("supports 1K targets", async function () {
});

test("supports 100K targets", async function () {
const impactedTargets = [...new Array(1_000)].map((_, i) => `target-${i}`);
const impactedTargets = [...new Array(100_000)].map((_, i) => `target-${i}`);
await runUploadTargets(impactedTargets);
expectImpactedTargetsUpload(impactedTargets);
});

test("supports IMPACTS_ALL", async function () {
const env = { ...ENV_VARIABLES, IMPACTS_ALL_DETECTED: "true" };
await runUploadTargets("ALL", env);
expectImpactedTargetsUpload("ALL");
});

test("rejects when missing API token", async function () {
await expect(runUploadTargets([], _.omit(ENV_VARIABLES, "API_TOKEN"))).rejects.toBeTruthy();
});
Expand Down