CI: logformatter: link to correct PR base

[edsantiago]

Two enormous misunderstandings:

1. $CIRRUS_BASE_SHA is worthless. I thought it was, you know, the BASE SHA of the current commit, but (as best I can tell) it seems to be the SHA of the most recent commit on the destination branch. Cirrus docs are unhelpful. Anyhow, it's clearly not anything useful. Stop using it.

2. $EPOCH_TEST_COMMIT is closer to what we want. It is defined in Makefile as the git merge-base. But for unknown reasons it was being clobbered in CI scripts, and it doesn't seem to work in all contexts, so, eliminate it from CI setup scripts. Leave it only in Makefile.

This leaves us with no option other than defining our own merge-base variable, PR_BASE_SHA. Do so and pass it along to rootless jobs.

Signed-off-by: Ed Santiago [email protected]

None

[edsantiago]

If this works, logformatted logs will link to 7b4f6ec as base, but CIRRUS_BASE_SHA will be fe18a87.

[Luap99]

Thanks I assume this is in reply to #23068 (comment)
I also couldn't find anything documenting the purpose of CIRRUS_BASE_SHA.

I do wonder now should this not remove all CIRRUS_BASE_SHA references from lib.sh, per comment it was only set there to use it for EPOCH_TEST_COMMIT. Now that you removed that part I would assume we can remove the entire block there.

[edsantiago]

Yes, thank you for reporting that.

I left CIRRUS_BASE_SHA because of the comment mentioning hack/get_ci_vm.sh. I don't know how that script works, and looking into it would require poking down a cascade of other repos. @cevich do you know what that comment is in reference to? Can you check if it's safe to completely eliminate CIRRUS_BASE_SHA?

[cevich]

I also couldn't find anything documenting the purpose of CIRRUS_BASE_SHA.

This value is only relevant for PRs. It's simply the sha of the branch the PR derived from. Since it's coming from the CI system, I considered it more authoritative than a value derived from git merge-base. As Paul mentions, it was used to set EPOCH_TEST_COMMIT because the (ancient) vbatts/gitvalidate tool we depended on was sensitive to that envar. I think there were a few places in the Makefile that also relied on an accurate EPOCH_TEST_COMMIT in a CI context.

I do wonder now should this not remove all CIRRUS_BASE_SHA references from lib.sh, per
comment it was only set there to use it for EPOCH_TEST_COMMIT.

As-is today, it wouldn't surprise me if many/most of these original use-cases have been moved/removed.

comment mentioning hack/get_ci_vm.sh

This isn't too hard to get your head around 😉

1. c/automation hosts cirrus-ci_env, a python mini-implementation of a .cirrus.yml parser. It's only job is to extract all envars given a task name (including from a matrix).
2. c/automation_images hosts get_ci_vm, this bundles cirrus-ci_env with an entry-point script.
3. When a user runs hack/get_ci_vm.sh there are NO magic variables set. The hack script simply runs what is defined as the repo's "setup" steps. You can see/check what it does by examining the --setup condition.
4. The only really complex part of c/automation_images get_ci_vm is the VM provisioning/orchestration.
5. Each repo's "setup" steps are then required to define any missing/needed CIRRUS_* variables. And ensure the values persist (normally by adding them into /etc/ci_environment on the VM after it's provisioned.

So, the hack script only indirectly depends on any CIRRUS_* values insofar as the rest of the repo. scripts require them. Even better, because the hack script uses the current (local) repo state, you can easily test "setup" changes by just using the tool to run a few tasks 😄

Edit: Opened containers/automation_images#362

[edsantiago]

[CIRRUS_BASE_SHA is] simply the sha of the branch the PR derived from.

It actually isn't: that's the purpose of this PR.

I interpret your comments as meaning "we don't care about CIRRUS_BASE_SHA" so I've removed it and re-pushed

[cevich]

it seems to be the SHA of the most recent commit on the
destination branch.

I'm misunderstanding something. Are you saying if a branch has commits 1, 2, and 3. CI running on a PR based on commit 2, will have CIRRUS_BASE_SHA set to commit 3?!?!?!?!

If so, that is indeed astonishing. To the point I would call it a Cirrus-CI bug.

meaning "we don't care about CIRRUS_BASE_SHA"

We only care about it to the extent that all of the other CI scripts and Makefile and whatnot care about it. The hack/get_ci_vm.sh script should be "mostly" decoupled from the specifics of CI in each repo. The only places where tight coupling can occur is in the --setup conditional block of the script.

That said, the CI simulation isn't perfect and interactions w/ lib.sh can be complex/unexpected. So it would be good to manually test a few tasks w/ the hack script to make sure CIRRUS_BASE_SHA really doesn't matter. I would suggest a 'build' and 'validate' tasks test run.

[edsantiago]

As I'm understanding it, CIRRUS_BASE_SHA means "whatever is the most recent commit on main at the time you submit your PR, no matter what your PR is branched from":

A---B---C---D---E    <--- HEAD on main
         \
          PR    <--- CIRRUS_BASE_SHA = E

Then if time passes, three more PRs merge into main (F, G, H), and you repush your PR without rebasing, CIRRUS_BASE_SHA = H

[cevich]

CIRRUS_BASE_SHA = H

Ewww, yes, that is indeed broken. I would/have expected it to remain "C" in your example, regardless of what happens on main.

I'll report this to Cirrus as a bug and if they can't/won't fix it, recommend they update the docs instead. "Base SHA if current build was triggered by a PR" isn't specific enough regardless.

Edit: Filed cirruslabs/cirrus-ci-docs#1279

[Luap99]

LGTM

[edsantiago]

Thanks, but it's not working! It's skipping all CI tests. EPOCH_TEST_COMMIT must be wonky but I can't find an envariable dump in which to see what it is. Marking draft to avoid merging this yet.

[edsantiago]

After much hairpulling, I believe this is working and ready for review. Sample log, scroll to top and note the Base commit link.

[edsantiago]

As best I can tell, PRE_COMMIT does not use EPOCH_TEST_COMMIT. This echo was added in #5066, back in February 2020, but I think it was a mistake.

[cevich]

I'm pretty sure pre-cmmit doesn't. I know vbatts/gitvalidate does use it though (in case that matters here).

[edsantiago]

debugging printf. Does anyone mind if I leave it in for a little while?

[cevich]

It's fine IMO

[Luap99]

LGTM

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Luap99,edsantiago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[cevich]

/lgtm