Check commits outside pr

[daixiang0]

See https://github.com/dapr/cli/runs/2970646884?check_suite_focus=true

/cc @fkirc

[fkirc]

I am not sure what this is about, more details might be needed.

[daixiang0]

There are two commits in the log:

Commit https://github.com/dapr/cli/commit/4e54707e77ff1c47cc9f2530668c739e89af0b85 is path-ignored: All of '.github/workflows/dapr_cli.yml,.github/workflows/installdaprwin.yml,.github/workflows/kind_e2e.yml,.github/workflows/release_notification.yml,.github/workflows/self_hosted_e2e.yml,.github/workflows/upgrade_e2e.yml' match against patterns '**.md,.codecov.yaml,.github/**/**.yml,CODEOWNERS'
Stop backtracking at commit https://github.com/dapr/cli/commit/0a37376658ef83e8a1b49178cf4e6f1c93eb1dfc because 'README.md,cmd/run.go,pkg/standalone/run.go,pkg/standalone/run_test.go' are not skippable against paths '' or paths_ignore '**.md,.codecov.yaml,.github/**/**.yml,CODEOWNERS'

While this pr only contains one. @fkirc

[thadamski]

Love the action! I'm experiencing the same problem with commits being referenced outside of the scope of commits of my PR.

My PR against develop has a single commit "abc123", however, the output of my skip_check job is finding a matching changed file in a grandparent commit, "abc125", which is outside the scope of the PR.

Commit lineage:

abc123 <- Commit in my PR
abc124 <- HEAD of develop
abc125 <- Parent of HEAD of develop. This has a matching file change, but isn't in my PR

I have an action configured to run on on a pull_request trigger against my primary branch develop:

name: My Action

on:
  pull_request:
    branches:
      - develop
  workflow_dispatch:

jobs:
  skip_check:
    runs-on: ubuntu-latest
    outputs:
      should_skip: ${{ steps.skip_check.outputs.should_skip }}
    steps:
      - id: skip_check
        uses: fkirc/skip-duplicate-actions@master
        with:
          paths: '["path/to/my/files/**"]'
          cancel_others: 'true'  # Cancel in progress run if new matching commit pushed after PR opened
  my-job:
    name: Some Job Name
    needs: skip_check
    if: ${{ needs.skip_check.outputs.should_skip != 'true' }}
    runs-on: ubuntu-latest
    steps:
      - name: Do Something
        run: |
          echo "Doing something"

[thadamski]

I think it might have something to do with the backtracePathSkipping method, which looks to always check the parent of the commit within the context, even if the parent is outside the scope of the branch for the PR. Is there a way to assert iterSha is one of the commits within the PR? I'm not too familiar with the octokit sdk... It still seems like you need to traverse to the parent commit since you may submit a PR with multiple commits, or push to origin w/ multiple local commits. Maybe commits direct to the main branch should also have a scope check if possible, maybe Github is aware of the batch of commits in a single push, if so that could be the scope for that scenario.

async function backtracePathSkipping(context: WRunContext) {
  let commit: ReposGetCommitResponseData | null;
  let iterSha: string | null = context.currentRun.commitHash;
  let distanceToHEAD = 0;
  do {
    commit = await fetchCommitDetails(iterSha, context);
    if (!commit) {
      return;
    }

    exitIfCommitOutOfScope(commit, context);  // <- Proposed new check
    exitIfSuccessfulRunExists(commit, context);

    if (distanceToHEAD++ >= 50) {
      // Should be never reached in practice; we expect that this loop aborts after 1-3 iterations.
      core.warning(`Aborted commit-backtracing due to bad performance - Did you push an excessive number of ignored-path-commits?`);
      return;
    }

    iterSha = commit.parents?.length ? commit.parents[0]?.sha : null;
  } while (isCommitSkippable(commit, context));
}

/** Exit with shouldSkip:true if the commit is out of the scope of the PR or branch */
function exitIfCommitOutOfScope(commit: ReposGetCommitResponseData, context: WRunContext) {
  const outOfScope = null;  // TODO: Check commit within the list of commits within the PR or branch

  if (outOfScope) {
    core.info(`Skip execution because all changes within branch are in ignored or skipped paths`);
    exitSuccess({ shouldSkip: true });
  }
}

function exitIfSuccessfulRunExists(commit: ReposGetCommitResponseData, context: WRunContext) {
  const treeHash = commit.commit.tree.sha;
  const matchingRuns = context.olderRuns.filter((run) => run.treeHash === treeHash);
  const successfulRun = matchingRuns.find((run) => {
    return run.status === 'completed' && run.conclusion === 'success';
  });
  if (successfulRun) {
    core.info(`Skip execution because all changes since ${successfulRun.html_url} are in ignored or skipped paths`);
    exitSuccess({ shouldSkip: true });
  }
}

[fkirc]

Thanks for the detailed description.
Just to make sure that I understand it correctly:
You want this Action to skip if an entire PR is in ignored paths, regardless of whether your "primary branch" has any successful tests in its history?
In other words, you want to look at the PR-diff and do not care about any tests that have been going on on the "primary branch" or other branches?

If this is what you want, then I believe that the entire backtracking-logic might be unnecessary since it might be easier to look at the PR-diffs (the same PR-diff that you use during manual code-review of PRs).

[thadamski]

You want this Action to skip if an entire PR is in ignored paths, regardless of whether your "primary branch" has any successful tests in its history?

Yes, that sounds like the use-case I was after! Something like a quality gate before code is allowed to be merge to trunk

[fkirc]

You want this Action to skip if an entire PR is in ignored paths, regardless of whether your "primary branch" has any successful tests in its history?

Yes, that sounds like the use-case I was after! Something like a quality gate before code is allowed to be merge to trunk

Alright, I think that I now understand it fully.
This behavior is currently not supported because the current philosophy is to build "proof chains" where skipped checks are based on older successful checks.

Nevertheless, I would be willing to merge a PR that implements this behavior for "pull_request"-triggers.
Perhaps it is simple to implement if the "commit-object" that comes from GitHub's REST API has enough information.

[thadamski]

The proof-chain sounds like a good idea and is probably a better solution than what I was proposing...I feel like I might have been experiencing unexpected scope because it was the first run, but I suspect follow-up runs will sort themselves out. I will keep you posted!

[paescuj]

If someone is still interested in such a feature, please consider funding it.