further clarifications and updates to the proliferation design doc

Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>

design/20230302.gomod.md

@@ -10,7 +10,7 @@ The intention here is to describe what we did and what we discovered, with an ey
 
 - It's hard or impossible to upgrade our dependencies months after a release
 - We won't change our conservative approach to backports
-- Having fewer dependencies in our go modules is a good thing
+- The fewer dependencies a go module has, the easier it is to maintain
 - It's OK if people can't import our binaries as go modules
 
 ### Solution
@@ -20,18 +20,26 @@ The intention here is to describe what we did and what we discovered, with an ey
 - Utilise local replace statements where possible
   - i.e. Binaries have a local replace for the core cert-manager module
   - This breaks imports of those binaries but means changes only require one PR
+- We call `github.com/cert-manager/cert-manager` the **core module**
+- We call all other new modules **secondary modules**
 
 ### Pros
 
 - Each binary can be patched independently
   - Side effects of a patch are limited to one binary when only that binary has the dependency
+    - For example, consider updating Helm before go module proliferation
+    - Updating the Helm version alone won't affect anything which doesn't import Helm
+    - **But:** Updating Helm also brings in Helm's updated dependencies which _would_ affect other binaries
+    - E.g. we and Helm depend on the k8s libraries
+    - That means that bumping Helm forces a bump of all k8s APIs for _all_ binaries
+    - With proliferation, bumping Helm would still bump the k8s libraries - but _only_ for cmctl!
   - This includes forking a dependency or needing to `replace` one
-  - Gives us more control over our own destiny
+  - In summary: Proliferation gives us more control over our own destiny
 
 - Core go.mod dependencies are reduced
   - All importers of `github.com/cert-manager/cert-manager` have fewer transitive dependencies
   - Reduced chance of dependency conflicts for all importers
-  - Including us - in subprojects!
+    - Including us - in our subprojects!
   - Many people need to import cert-manager! (pkg/apis, etc).
   - We might split things more in the future - this is a good first step
 
@@ -42,11 +50,11 @@ The intention here is to describe what we did and what we discovered, with an ey
 
 ### Cons
 
-- Using local `replace` statements for binaries will break imports for those binaries
-  - We assume this won't be too destructive in most cases
+- Using local `replace` statements for binaries will break external importers of those binaries
+  - We assume this won't be too destructive in most cases (since we don't see many importers of those binaries)
   - If we need to make binaries importable again, we can change them to use regular import statements
-  - That would require two PRs in the event that we need to change the binary + the core module at the same time
-  - If the binary would've ended up in a separate repo anyway (e.g. cmctl) we'd have done this eventually
+  - That would require two PRs in the event that we need to change the secondary module and the core module at the same time
+  - If the secondary module would've ended up in a separate repo anyway (e.g. cmctl) we'd have done this eventually
 
 - Increased complexity in working with the codebase
   - E.g. `go test ./...` no longer tests _everything_, since it won't recurse into modules
@@ -98,13 +106,16 @@ We can create several new Go modules so that each binary we build can have disti
 `cmctl` having a dependency on Helm would only affect `cmctl` and wouldn't force us to change any of the other
 components we build in order to patch a Helm vulnerability.
 
-In addition to separating out the binaries, we could also separate out the "core" issuer implementations into a
-distinct module (since they bring so many external dependencies and currently live in `pkg/` where they'll be part of
-the core cert-manager `go.mod` file).
-
 Plus, where we have testing-only dependencies (e.g. for integration or end-to-end tests) we could create a test module
 so that those test dependencies don't pollute the main `go.mod`.
 
+### Terminology
+
+Currently cert-manager has one module name: `github.com/cert-manager/cert-manager`. This import path is widely used and
+we can't break imports of this module. We'll call this the **"core" module.**
+
+This proposal also introduces several new modules which depend on the core module. We'll call these "secondary" modules.
+
 ### Solution Detail
 
 First, we'll add a go.mod file for each binary we ship under `cmd/` - `acmesolver`, `cainjector`, `controller`, `ctl` and `webhook`.
@@ -143,37 +154,10 @@ cmd
 
 These changes will also require tweaks to how modules are built and tested, which will be done in our `Makefile`.
 
-Plus there are some imports of `cmd` under `test/` which will need to be changed or updated.
-
 After these changes, running `go mod tidy` on the core cert-manager module should clean a lot of dependencies but will
 leave many SDKs since they're depended on by issuer logic which is in `pkg/`.
 
-Next, we'll abstract this issuer-specific logic into its own module. In the Hackathon where this concept was originally
-explored, we named this module `intree` (as in, in-tree issuers). Any references to this code will be removed from `pkg`,
-which should remove dependencies on cloud SDKs from cert-manager's core module, leaving them only in the new `controller`
-module (note that separating these issuers into a separate binary is a separate proposal. This proposal keeps those issuers
-in the `controller` component for now).
-
-```text
-intree
-└── issuers
-    ├── acme
-    │   ├── ...
-    │   ├── go.mod
-    │   ├── go.sum
-    │   └── issuer
-    │       └── ...
-    ├── vault
-    │   ├── ...
-    │   ├── go.mod
-    │   └── go.sum
-    └── venafi
-        ├── ...
-        ├── go.mod
-        └── go.sum
-```
-
-There'll also be some test imports at this point which will need to be moved around (some detail below).
+As part of this process there will be several import paths which will need to be fixed, but nothing should break.
 
 ### Workflow Example: Changing a Binary
 
@@ -181,10 +165,10 @@ NB: See `Importing cert-manager / Development Experience` below for an explorati
 behind the proposed solution.
 
 As an example of the kind of change being discussed, imagine adding a new field to our CRDs along with a feature gate. This
-would require changes both to the binaries (e.g. the controller) and to the core cert-manager packages.
+would require changes both to at least one secondary module (e.g. the controller) and to the core cert-manager module.
 
 In order to avoid having to make two PRs for this kind of change we propose to explicitly state that any external import of
-any of the new modules under `cmd` is not supported. By breaking this kind of external import, we can  use the `replace` directive
+the new modules under `cmd` is not supported. By breaking this kind of external import, we can use the `replace` directive
 in the new `go.mod` files for each of the binaries to refer to the cert-manager package in the same repository.
 
 This means that every change to `pkg/` will automatically be picked up by all of the binaries that we build and test in CI.
@@ -211,7 +195,7 @@ module - or anyone who was previously importing `github.com/cert-manager/cert-ma
 
 ### Importing cert-manager / Development Experience
 
-**In short:** Module replacements help developers but don't aren't respected by imports, meaning some changes could need two PRs or we'd have to break anyone importing certain modules
+**In short:** Module replacements help developers but aren't respected by imports, meaning some changes could need two PRs or we'd have to break anyone importing certain modules
 
 **Useful Reference:** It helps to read [this StackOverflow comment](https://stackoverflow.com/a/71984158) to better understand the options we have
 
@@ -228,20 +212,17 @@ For example we could look at the `controller` component which would depend on th
 An issue with this approach is that the `replace` statement wouldn't be respected if anyone imports the controller module
 from a 3rd party project. Instead, that 3rd party would see an error relating to an unknown version of cert-manager.
 
-For `cmd/controller` it might be acceptable for us to break 3rd party imports - that's a decision we'd have to make -
-but for other modules that might not be reasonable. For example, if we created a new module for each of the built-in
-issuers, it would depend on the core cert-manager module but would also need to be imported by the `controller`
-component.
+For this example involving `cmd/controller` it might well be acceptable for us to break 3rd party imports but for other
+modules that might not be reasonable. In that case, we'll always have a fallback; using a 'regular' import of the core module.
 
-**In that case** we'd not be able to use replace directives and we'd need to refer to specific cert-manager versions.
-That would in turn mean that in order to make a change to both the issuer module and the core module, we'd have to first
-create a PR for the core module, get that merged and then create a PR for the issuers module which includes an update
-to the issuers `go.mod` to point at the new cert-manager version.
+This would mean that we create two PRs for a change; the first changes the core module, and the second updates the secondary
+module to import the new core module version created by the previous PR.
+
+UPDATE: As we implemented this design, it was decided that we didn't want to break imports of `cmctl` because it was
+used in several other cert-manager subprojects - so cmctl uses the approach described above.
 
 #### Potential Solution for Developer Experience: Dynamic `go.work`
 
-(Untested)
-
 We could introduce a make target which generates one or more `go.work` files locally to point all modules at local
 development versions. This doesn't help with having to raise two PRs for a change, but it does help minimise the
 burden of testing changes locally.
@@ -327,7 +308,7 @@ There's probably some low hanging fruit we could pick here, but we're unlikely t
 of our dependencies. That means the problem won't go away - and there's always the chance that we need to add new
 dependencies down the road.
 
-## Stretch Goal: `pkg/apis` Module
+## Addendum: Groundwork for `pkg/apis` Module
 
 We've talked before about creating a separate `pkg/apis` module or repo to improve the experience for users who need
 to import that specific path (which is common).
@@ -335,7 +316,8 @@ to import that specific path (which is common).
 Module proliferation could be a solution here by making that path a new module.
 
 Changing the `pkg/apis` module isn't really related to reducing dependencies so it's a little different to the rest of
-this proposal but it might be an easy solution.
+this proposal and we don't propose to do it as part of this work. But the implementation of this design might inform
+how we could approach solving the `pkg/apis` problem.
 
 ## Footnotes