[release-v1.17] fix: PaC does not refer tekton hub

[matejvasek]

• Removed dependency on Tekton Hub that no more exists.
• Adjusted testing env, in particular testing KinD cluster setup.
  • Set up RH pull secrets for KinD as we use RH images.
  • Install the git-clone task into the KinD cluster so it's at same place as in OCP.

[twoGiants]

Great that you removed the hub reference! 😸 👍

Now there is new logic with error handling. Usually I'd say this needs unit tests to cover it. Wdys in this case?

[matejvasek]

@jrangelramos please verify that this works.

[matejvasek]

Great that you removed the hub reference! 😸 👍

Now there is new logic with error handling. Usually I'd say this needs unit tests to cover it. Wdys in this case?

@twoGiants
I replied it the other comment. I would add test should this be in "main". This is however just quick fix for 1.37.1 patch release. The release branch is pretty much inactive and there most likely won't be 1.37.2. So such a test won't catch any regression, since nobody would run it.

Wrt test for errors in general: I do not find it very useful for "non actionable/recoverable" errors. I mean errors where there is nothing to be done -- you either interrupt/crash whole app or just print warning. OK I mean it's good to check we are not ignoring any error but it is quite well done in Go IMO. Go forces you to check errors by design, so there is little change there is unhandled error.

However I think they are very much useful when we return "sentinel" error which value is subsequently test using errors.Is() and do some dependent action.

[jrangelramos]

it works. it looks good to me

[twoGiants]

... release branch is pretty much inactive and there most likely won't be 1.37.2. So such a test won't catch any regression, since nobody would run it.

Fine with me then! 👍

Wrt test for errors in general: I do not find it very useful for "non actionable/recoverable" errors. I mean errors where there is nothing to be done -- you either interrupt/crash whole app or just print warning. OK I mean it's good to check we are not ignoring any error but it is quite well done in Go IMO. Go forces you to check errors by design, so there is little change there is unhandled error.

Not sure I understand correctly => returning an error where there was no such an operation before looks actionable to me BUT I might misunderstand what you want to say. I'll write you a PM, no need for a Github comment discussion 😆

However I think they are very much useful when we return "sentinel" error which value is subsequently test using errors.Is() and do some dependent action.

Agree 👍

[twoGiants]

/approve

Let me know when the tests are fixed so I can lgtm it! 👍

[matejvasek]

rebased

[matejvasek]

@twoGiants lgtm plz

[matejvasek]

@twoGiants lgtm plz

Ah not yet some test are failing.
Or maybe I could override these and fix them in separate PR?

[matejvasek]

Ah not yet some test are failing.

might be a flake

[matejvasek]

@twoGiants I added fix up to our testing setup so event the KinD cluster have the Openshift Pipelines specific task.

[matejvasek]

@twoGiants all tests are passing.

[twoGiants]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek, twoGiants

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [matejvasek]

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