ci: update files diff method in pipeline

[yihau]

Problem

$(gh pr view "$pr_number" --json commits --jq '.commits | length') will only return 100 files...

Summary of Changes

fix it!

(this one should be able to fetch up to 3,000 files) should be able to fetch all affected files back

[LucasSte]

Does it work if we have more than 20 thousand lines in a diff?

[yihau]

@LucasSte yes. I guess you're referring to #1850.

gh api graphql -f query='
  query($owner: String!, $repo: String!, $pr: Int!, $endCursor: String) {
    repository(owner: $owner, name: $repo) {
      pullRequest(number: $pr) {
        files(first: 100, after: $endCursor) {
          pageInfo{ hasNextPage, endCursor }
          nodes {
            path
          }
        }
      }
    }
  }' -F owner='anza-xyz' -F repo='agave' -F pr=1850 --paginate --jq '.data.repository.pullRequest.files.nodes.[].path' | wc -l

68

^^^ it can get all correctly

[illia-bobyr]

I think it is a good improvement.

I still think that it would be better to run all commands that have a higher change of failure with explicit exit code checking, as I've done in https://github.com/anza-xyz/invalidator/pull/359.

In my case, I do not care for the accuracy.
What I do care about is that a run is created.
It is fine if it is not 100% accurate.
But it is annoying if it errors out because the GitHub tool failed, and I get nothing at all.

I think we are looking at it from different angles.
Maybe after you merge this change, I can rebase my change on top and merge it as well.
Or I can merge my change first, and you rebase your improvement on top.

[illia-bobyr]

Do you know if we can get these from some environment variables set by Buildkite or something like that?

[yihau]

afaik we can parse it from BUILDKITE_REPO but I think we should be fine with current status 🤔

[illia-bobyr]

Is it non-trivial?
Not insisting, just wondering.
In general, the more constants we hard-code the more fragile things become.
But we obviously want to weight it against the parsing complexity.

And, in particular, we have other repos that use this code, that would fail to run this script correctly, if these values are hardcoded.

[yihau]

ff378db

[illia-bobyr]

👍 for using ShellCheck.

It is a good practice to explain why any of the checks are disabled, when they are.
Shellcheck documentation suggests it as well.
It is hard to know what is SC2016 or SC2046 without doing a search.

SC2016 is about accidentally using single quotes.
This is intentional, so it is nice to just say so, and not require the reader to use Google and guess.

SC2046 on the other hand is "Quote this to prevent word splitting" - which is a common source of subtle errors.
It is better not to disable it, and it does not seem to be necessary here.

Suggested change

	# shellcheck disable=SC2016
	# shellcheck disable=SC2046
	# ref: https://github.com/cli/cli/issues/5368#issuecomment-1087515074
	query='
	# ref: https://github.com/cli/cli/issues/5368#issuecomment-1087515074
	#
	# Variable value contains dollar prefixed words that look like bash variable
	# references. This is intentional.
	# shellcheck disable=SC2016
	query='

[yihau]

thank you! 22c3a6b

[illia-bobyr]

https://github.com/anza-xyz/agave/pull/2075/files

[illia-bobyr]

Is it non-trivial?
Not insisting, just wondering.
In general, the more constants we hard-code the more fragile things become.
But we obviously want to weight it against the parsing complexity.

And, in particular, we have other repos that use this code, that would fail to run this script correctly, if these values are hardcoded.