Revisit running gitlint in the CI

[phlogistonjohn]

After having to fight with actions/checkout in another repo for other reasons I learned a few things that may help us run the commit checks in the github ci

[phlogistonjohn]

@synarete what do you think? I think the big differences between this and #274 is that I added gitlint to be auto installed like many of the other tools and then the thing that "makes it work" in the CI is to use the ref argument to checkout action as well as using the origin/ prefix on the branch name.

[synarete]

@synarete what do you think? I think the big differences between this and #274 is that I added gitlint to be auto installed like many of the other tools and then the thing that "makes it work" in the CI is to use the ref argument to checkout action as well as using the origin/ prefix on the branch name.

@phlogistonjohn Installing gitlint locally (like we do with other bin tools) looks great. That said, make check-gitlint still fails when integrated with github actions. See: https://github.com/synarete/samba-operator/actions/runs/4649123528/jobs/8227255345

[phlogistonjohn]

@synarete what do you think? I think the big differences between this and #274 is that I added gitlint to be auto installed like many of the other tools and then the thing that "makes it work" in the CI is to use the ref argument to checkout action as well as using the origin/ prefix on the branch name.

@phlogistonjohn Installing gitlint locally (like we do with other bin tools) looks great. That said, make check-gitlint still fails when integrated with github actions. See: https://github.com/synarete/samba-operator/actions/runs/4649123528/jobs/8227255345

It's possible I overlooked something but I don't think the version you are testing is setting the ref parameter for actions/checkout@v3. I copy and paste the text below from the run you linked:

Run actions/checkout@v3
  with:
    fetch-depth: 0
    repository: synarete/samba-operator
    token: ***
    ssh-strict: true
    persist-credentials: true
    clean: true
    lfs: false
    submodules: false
    set-safe-directory: true
Syncing repository: synarete/samba-operator

Try setting ref: ${{ github.event.pull_request.head.sha }} as a sibling to fetch-depth: 0, and see if that improves things for you. Notice how in my run it sets ref and it only logs an error for the "bad news" commit - as that is an intential failing commit I use to check the check and I plan to remove before merging

[phlogistonjohn]

On second thought, I think I understand now. You're basing this run off of my commits but you are not running them as part of a PR. So while I think you have the key specified in the job, the value ${{ github.event.pull_request.head.sha }} maps to nothing because you are running it on a "raw" branch not a PR. So there's no pull_request sub section! We probably don't need this check on anything not a PR anyway so we can add if: github.event_name == 'pull_request' to the job so that it only runs if pull_request is going to have data in it.

[synarete]

@synarete what do you think? I think the big differences between this and #274 is that I added gitlint to be auto installed like many of the other tools and then the thing that "makes it work" in the CI is to use the ref argument to checkout action as well as using the origin/ prefix on the branch name.

@phlogistonjohn Installing gitlint locally (like we do with other bin tools) looks great. That said, make check-gitlint still fails when integrated with github actions. See: https://github.com/synarete/samba-operator/actions/runs/4649123528/jobs/8227255345

It's possible I overlooked something but I don't think the version you are testing is setting the ref parameter for actions/checkout@v3. I copy and paste the text below from the run you linked:

Run actions/checkout@v3
  with:
    fetch-depth: 0
    repository: synarete/samba-operator
    token: ***
    ssh-strict: true
    persist-credentials: true
    clean: true
    lfs: false
    submodules: false
    set-safe-directory: true
Syncing repository: synarete/samba-operator

Try setting ref: ${{ github.event.pull_request.head.sha }} as a sibling to fetch-depth: 0, and see if that improves things for you. Notice how in my run it sets ref and it only logs an error for the "bad news" commit - as that is an intential failing commit I use to check the check and I plan to remove before merging

@phlogistonjohn ref is properly set, see https://github.com/synarete/samba-operator/blob/ss-gitlint-review/.github/workflows/main.yml#L32-L42

Yor made a test with expected-to-fail commit, while I tried with should-not-fail commits. This is the same problem I encountered in my previous iteration with gitlint: when run via github actions make check-gitlint failed on valid commits (but does not happen on local repository).

[synarete]

On second thought, I think I understand now. You're basing this run off of my commits but you are not running them as part of a PR. So while I think you have the key specified in the job, the value ${{ github.event.pull_request.head.sha }} maps to nothing because you are running it on a "raw" branch not a PR. So there's no pull_request sub section! We probably don't need this check on anything not a PR anyway so we can add if: github.event_name == 'pull_request' to the job so that it only runs if pull_request is going to have data in it.

@phlogistonjohn Good catch!

[synarete]

LGTM

[anoopcs9]

/retest centos-ci/sink-clustered/mini-k8s-1.26

[anoopcs9]

We probably don't need this check on anything not a PR anyway so we can add if: github.event_name == 'pull_request' to the job so that it only runs if pull_request is going to have data in it.

What happened to this part? Don't we need to differentiate pull_request and other events?

[phlogistonjohn]

We probably don't need this check on anything not a PR anyway so we can add if: github.event_name == 'pull_request' to the job so that it only runs if pull_request is going to have data in it.

What happened to this part? Don't we need to differentiate pull_request and other events?

Hah. I apparently added it to the sambacc version instead of this PR:
samba-in-kubernetes/sambacc@dda4f72#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR15

Will fix.

[obnoxxx]

looks good in general but found one typo ...

[phlogistonjohn]

/retest centos-ci/sink-clustered/mini-k8s-1.26

[obnoxxx]

typofix mentioned previously

it has been addressed

[phlogistonjohn]

Storing signatures
kustomize not found in PATH, checking /root/samba-operator/.bin
make: *** [Makefile:256: kustomize] Error 1
Build step 'Execute shell' marked build as failure
Performing Post build task...

I don't see how my change would have broken the install for kustomize... but I did touch the file that makes it happen.
I tried running it locally and it succeeded.

[obnoxxx]

after more careful review, the PR lgtm, but a ci check is failing. this needs to be fixed to meet all requirements

[anoopcs9]

Storing signatures
kustomize not found in PATH, checking /root/samba-operator/.bin
make: *** [Makefile:256: kustomize] Error 1
Build step 'Execute shell' marked build as failure
Performing Post build task...

I don't see how my change would have broken the install for kustomize... but I did touch the file that makes it happen. I tried running it locally and it succeeded.

Even my local run completed successfully with the patch. I'll take a look at it later.

[phlogistonjohn]

I fixed the CI failure. install-tools.sh broke silently when either go (not in this case) or python3 (which failed in this case) were not on the system it also didn't matter if that was being used or not, so I updated the script with a 2nd patch.

[anoopcs9]

lgtm.

[anoopcs9]

I fixed the CI failure. install-tools.sh broke silently when either go (not in this case) or python3 (which failed in this case) were not on the system it also didn't matter if that was being used or not, so I updated the script with a 2nd patch.

Awesome find 👏🏻

Especially I liked the part where you added another commit instead of squashing the dependency change into previous one which kind of helped me easily understand the "missing python3" error in CentOS CI.

[obnoxxx]

lgtm