Improve package CI error handling

[SgtCoDFish]

Fixes #246 - see that issue for details!

[SgtCoDFish]

/test pull-trust-manager-verify

[erikgb]

I think this is a real flake, as I have seen it a couple of times already - both locally and in CI.

Integration [It] should migrate configmap from CSA to SSA
/home/prow/go/src/github.com/cert-manager/trust-manager/test/integration/bundle/suite.go:480
  [FAILED] Unexpected error:
      <*errors.StatusError | 0xc000628f00>: 
      Operation cannot be fulfilled on bundles.trust.cert-manager.io "test-bundle-4nxpl": the object has been modified; please apply your changes to the latest version and try again

To me, this seems like some kind of race condition between the controller and the test.

[SgtCoDFish]

/test pull-trust-manager-verify

[SgtCoDFish]

To me, this seems like some kind of race condition between the controller and the test.

Wouldn't surprise me 🙃 I'd like to rip the whole test suite to pieces and start fresh tbh!

[erikgb]

I'd like to rip the whole test suite to pieces and start fresh tbh!

Why? What would you do? I think these tests with a simplified control-plane are very valuable. But I know that the cert-manager team prefers unit tests. Yes, unit tests run a little faster, but tests using the control-plane are better IMO. 😉

[SgtCoDFish]

Why? What would you do? I think these tests with a simplified control-plane are very valuable. But I know that the cert-manager team prefers unit tests. Yes, unit tests run a little faster, but tests using the control-plane are better IMO. 😉

Oh on the contrary, we much much prefer integration + e2e tests by a lot!

I'd like to remove Ginkgo because I think it makes tests more confusing to reason about. I've also been working on something (closed source but hoping I can open it for the cert-manager project) which manages creating the control plane / kind clusters in the test itself, which is much more efficient, easier to handle, and much more parallellisable!

That's a thing for another time though!

[erikgb]

I am not a basher, so I'll let someone else review this - thought the changes look ok to me. But I wonder why we seem so "scared" of pushing the trust image? Even pushing over an existing tag seems fine to me, since the image build should be reproducible IMO. So there must be a reason. 😉

[SgtCoDFish]

But I wonder why we seem so "scared" of pushing the trust image? Even pushing over an existing tag seems fine to me, since the image build should be reproducible IMO. So there must be a reason.

Pushing over the existing tag is what we did yesterday so it does work 😁

The reason we have the check is that we run this CI every couple of hours so we can update quickly when something changes upstream, and it takes about 1 min to run if we don't build and it takes about 23.5 mins to run if we build everything and push. So there's a cost savings element (not huge - maybe $1 a day - but it's not nothing) and there's a "not pushing loads to quay.io" element (as quay.io can be flaky and we want to be good citizens!)

[inteon]

Looks good to me @SgtCoDFish, as mentioned before, the image overwrite probably is not a big deal, but we want our scripts to behave more predictably anyway.
/approve
/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

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 [inteon]

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