Why the amount of service types a limit? I think the benefit is that the implementation is dynamic right?

If we pass more secret names to newFixture than the size of serviceTypes then serviceTypes[i] will get out of bounds, this is a check to ensure we don't write tests wrong. We could change newFixture to replace secretNames ...string with a map[storage.ServiceType]string to prevent that, but newFixture is just a quick test utils function so I wanted to make it easy to call.
We cannot use serviceTypes[i %len(serviceTypes)] because then we would be overriding the value for a service type in the secretsMap map[storage.ServiceType]*v1.Secret that the repo is persisting

I've finally removed the s.Require().LessOrEqual because newFixture now only accepts a single secret name, which is what the tests are using. See next comment for more context

I would like to be more explicit here. It's a bit off that the first element of the given string array represents a specific service. Also your tests only ever use the first element.

when I wrote newFixture I wasn't sure how many secrets we'd be adding to the fake client set. So I'll rewrite this so newFixture accepts a single secret name, and simplify the creation of these maps removing the look and the s.Require().LessOrEqual above. This should make the test easier to read

Why is this a problem? Perhaps the caller does not want to update all previously declared secrets this time?
What worries me more is that we're silently ignoring entries in secrets that were not declared at repo initialization time.

To be honest I don't understand this, can you explain why this is necessary?
I would expect the secrets repo knows how to convert between v1.Secret <> storage.TypedServiceCertificateSet and communicating to the k8s API.

This is to ensure the invariant "All secrets with non nil Data store the same CA PEM." A difference between storage.TypedServiceCertificateSet and v1.Secret is that the same CA is repeated in different secrets, while it appears only once in storage.TypedServiceCertificateSet.
But on second though that check is not really needed here because we just use secrets for the ObjectMeta but we ignore the Data in those secrets. So I'll remove it from here and turn it into an error in getServiceCertificates when different CAs are found for different secrets

Also I think the repository can be initialized with all service types currently supported instead of passing them directly to it.

The idea here is that the repository will use secrets as a template for storing secrets in k8s, and it will only modify the secret Data with the certificates received in storage.TypedServiceCertificateSet. That way the sensor component can handle the logic of setting up ObjectMeta with suitable OwnerReferences, and the repository doesn't care about that.
This is documented in the invariant "secrets and secretsClient are read-only, except the field Data of the entries in secrets" in the docstring for type serviceCertificatesRepoSecretsImpl

rename done

Why checking here the secrets? The repo should be stateless and not know about the underlying data.
Imho it is up to the application layer to ensure that the CA cert are configured correctly.

Same reason as I mentioned above: we are using the secrets as templates, be able to use the secret names for reading from k8s and to persist in k8s with the right ObjectMeta

I lost track on the discussions, so commenting here.
Why should the serviceCertificateSecret be passed to the repo in the constructor?
To me it provides no value (expect DI) because we know upfront which storage.ServiceType are supported and which not.
Now every time the repo is used I need to construct the map[storage.ServiceType]serviceCertificateSecret and pass it to the repo? 🤔

I still think this should only be implemented for scanner certs for now and avoiding pre-optimizations.

The repo currently only wraps the k8s api and adds type conversion logic, for this the current implementation introduces complexity which is not necessary.
Let's keep it simple, if we need to support other services we still can add them.

Why should the serviceCertificateSecret be passed to the repo in the constructor?

As discussed in #457 (comment) and #457 (comment) this repository is only concerned with modifying the secret data, not with creating secrets, or the secret name or ownership. By keeping that responsebility out of the repo we are making the repo simpler.
serviceCertificateSecret contains that information which is not created by the repository as a *v1.Secret, and also the paths for the certificates in the secret. All that information is constant for the life of the repository and that is why it is passed in the constructor. That way getServiceCertificates and putServiceCertificates have less parameters and are easier to use, and harder to use wrong.

we know upfront which storage.ServiceType are supported and which not

I understand that you mean we could instead pass two serviceCertificateSecret one for scanner and another for scannerDB, or we could use instead

type serviceCertificateSecrets struct { 
    scannerSecret serviceCertificateSecret
    scannerDBSecret serviceCertificateSecret
}

That is the same information in map[storage.ServiceType]serviceCertificateSecret, but with a type that is less flexible. I can change map[storage.ServiceType]serviceCertificateSecret to accept two serviceCertificateSecrets, but I see no value in changing the field secrets map[storage.ServiceType]serviceCertificateSecret in serviceCertificatesRepoSecretsImpl.

expect DI

I don't know what "DI" means, can you please explain?

DI = Dependency Injection

I still think this is not necessary, instead a valid secret must be passed on every put to the repo.
If the ownerRef is necessary it should be injected instead.

requiring passing additional parameters that would be constant for the life of the repository would make putServiceCertificates harder to use, which might lead to more errors

Use secretClient.Create directly here, no need to check existence beforehand.

this comment seems to be outdated, at 8456e48 createSecret uses Create directly

Return error here if secrets were not owned by the deployment. The GetServiceCertificates is then used to determine whether the ticker stops or not.

this comment seems to be outdated, at 8456e48 getServiceCertificates returns ErrUnexpectedSecretsOwner if the owner is not as expected

fixed

I think this doc line can be removed, at least to me it is clear that this happens in all cases where a connection is necessary.

done

Why not using multierror and returning all errors at the same time?

I just investigated and I learned that errors.Is can be used to determine whether or not an error appears inside a multierror, so I'll follow your suggestion and use errors.Is in the client code

Named params please

done

named params please

done

Why should it fail here? Can you described that in the test description?

These errors are forced in the k8s API mock, by passing "patch" to the verbToError parameter of newFixture, which uses clientSet.CoreV1().(*fakecorev1.FakeCoreV1).PrependReactor to force the error for the verb. I've renamed the test name here to "failed put due to k8s API error", and also in TestGet that was doing something similar

Why did you created this function?
If errors are checked I expected either checking the type with s.ErrorIs or if not available checking the error msg with s.Contains(err.Error(), "expected string").

replaced with s.ErrorIs, nice to know about this

What I don't like about that signature is that verbToError magically gets empty strings passed which disturbes the reading flow of a test.
I need to first look at newFixtureAdvancedOpts and understand what's going on under the hood.

Can you use a struct as the input parameter which is more expressive instead of multiple parameters?
This makes it more obvious because the passed parameters become named automatically (or using constants for verbToError and emptySecretData).

I know a lot of our unit tests don't do that - and that's not good because it gets frustrating debugging these.

verbToError is the HTTP verb of the k8s API for which we force an error in the fake k8s client set, so by passing "" we specify that all verbs that we'll use will work fine.
I created a certSecretsRepoFixtureSpec struct as suggested, and unified newFixtureAdvancedOpts and newFixture, into just newFixture that takes that struct. I've also added comments to both newFixture and certSecretsRepoFixtureSpec to clarify how this behaves

I like the simplicity and readability of this function!