Mock DBClient with GoMock

[mbarnes]

What this PR does

• Introduces GoMock as a development tool, installed by Bingo.
• Generates a mocked DBClient implementation in a new "mocks" package under /internal/mocks.
• Converts existing unit tests to use the mocked DBClient instead of the in-memory cache DBClient.
• Removes the --use-cache command-line option.
• Finally, discards the in-memory cache DBClient.

The justification for all this is explained in the epic ARO-14167 - Use Cosmos DB's Transactional Batch Operations in RP.

Jiras:

• ARO-14189 - Track new mock CLI utilities in Bingo
• ARO-14168 - Use GoMock to mimic database operations in unit tests

Link to demo recording:

Special notes for your reviewer

The last commit was arrived at after much trial and error. I don't know if there's a better way to adapt the GitHub workflow -- I couldn't find any predefined actions for installing Bingo, nor could I suss out how ARO-RP does it -- so someone with more experience can feel free to correct me.

[github-actions]

Please rebase pull request.

[github-actions]

Please rebase pull request.

[mbarnes]

All review comments are addressed. I was able to simplify all the DoAndReturn calls to just Return.

[SudoBrendan]

Thanks Matt! A few more comments here.

[SudoBrendan]

I hope this isn't too much scope creep - but we've got new code that's auto-generated to consider in this PR, so a few things I want to note:

CONTEXT:

While I too was originally on-board with your pattern, where go generate commands ultimately get called as a direct dependency of their requirements, in Classic, we eventually made the decision not to do this for a few key reasons:

1. The golang maintainers say that go generate should not be part of your build process - it should be independent and committed as code (rather than generated on the fly).
2. In the case here - tests in CI might pass even though you didn't run go generate locally and committed the diff. This means that we haven't guaranteed the code in git is up-to-date with what is expected in a given commit on PRs. Instead, we should probably do the same in classic: if there's a diff after running go generate, that should produce a CI error.
3. This extends the duration of our test suite long-term, especially when we have quite a few mocks to generate in the long-game of this repo.

More of me being ripped tearing and screaming away from this pattern can be read here: https://redhat-internal.slack.com/archives/C02ULBRS68M/p1724353443729429?thread_ts=1724304200.163579&cid=C02ULBRS68M

WHAT I THINK WE SHOULD DO:

1. Remove mocks as a direct dependency of test, instead, the dev is expected to run make mocks if interfaces have changed.
2. Add CI logic to run an equivalent of make mocks && exit 1 on git diff to ensure that make mocks was run correctly before merge: https://github.com/Azure/ARO-RP/blob/b6a5f9ca0d2862ba3672271204659d835d91470b/.github/workflows/ci-go.yml#L29-L42

[mbarnes]

I removed mocks as a prerequisite for test in an existing commit and appended a new commit for a new "Regenerate mocks" CI step.

[mbarnes]

Also ran a test with a now-reverted commit where I modified /internal/mocks/dbclient.go and CI caught it.

[SudoBrendan]

nit: thinking long-game - do we potentially want to reflect the package directory structure inside of internal/mocks to prevent the mocks directory from being too much of a dumping ground? I would think we'd at least want something like:

./internal/
├── database
│   └── database.go
└── mocks
    ├── database
    │   └── dbclient.go
    └── generate.go

We could also consider renaming the mock file name to database.go so we know what file the interface mocks are from - which is unknown without reading generate.go?

./internal/
├── database
│   └── database.go
└── mocks
    ├── database
    │   └── database.go
    └── generate.go

Using this structure, our test suite would be doing something like:

import (
    "internal/database"
    db_mocks "internal/mocks/database"
)

Which feels more directed and obvious exactly what mocks we intend to use, instead of "the mocks"? For example, in a future PR, if I see an update to the internal/database tests, and it had:

  import (
      db_mocks "internal/mocks/database"
+     backend_mocks "internal/mocks/backend"
  )

I would immediately recognize that as a code smell in a way that wouldn't be recognized with the current structure.

[mbarnes]

I don't foresee us mocking much else beside maybe the ClusterServiceClient interface. I'm not opposed to the idea if it does start to become a dumping ground, but I think it's a little premature now.

[SudoBrendan]

and it looks like that is already mocked out by hand... ack