search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
243 stars 65 forks source link

Improve package CI error handling #247

Closed SgtCoDFish closed 8 months ago

SgtCoDFish commented 8 months ago

Fixes #246 - see that issue for details!

SgtCoDFish commented 8 months ago

/test pull-trust-manager-verify

erikgb commented 8 months ago

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 commented 8 months ago

/test pull-trust-manager-verify

SgtCoDFish commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

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!

SgtCoDFish commented 8 months ago

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 commented 8 months ago

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 commented 8 months ago

[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](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)~~ [inteon] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment