Import valid version of CAPI from test module

[g-gaston]

User Story

As a developer I would like to be able to import the test module, or other modules importing the test module, without the need of replace's in my go.mod

Detailed Description

The current test module imports the main CAPI module with version v0.0.0-00010101000000-000000000000. Even though this version doesn't exist, it doesn't cause any problem for local use/development of the test module since there is a replace directive pointing to local disk (replace sigs.k8s.io/cluster-api => ../). This is needed to facilitate CAPI development without the need to separate the main module and the test module changes in different commits.

However, when other modules import the test module, like some CAPI providers do (CAPV), this breaks the build, since replace directives are not inherited. In order to fix it, a manual replace directive is required in the importing go.mod.

This produces some weird situations like:

require(
  sigs.k8s.io/cluster-api v1.1.0
  sigs.k8s.io/cluster-api/test v1.1.0
)

replace sigs.k8s.io/cluster-api => sigs.k8s.io/cluster-api v1.1.0

One could argue that this is just fine for providers, since if they are importing the test module they are most probably importing the main CAPI module. And in that case, adding a replace directive for a module they are already importing is not a big deal.

However, this gets even trickier for 2nd tier importers, like a project that wants to import the CAPV project. In that case the solution can be something like

require sigs.k8s.io/cluster-api-provider-vsphere v1.0.2

replace sigs.k8s.io/cluster-api => sigs.k8s.io/cluster-api v1.1.0

or if they don't rely on that part of the codebase

require sigs.k8s.io/cluster-api-provider-vsphere v1.0.2

exclude sigs.k8s.io/cluster-api/test v1.1.0

So now not only the user needs to add extra directives to their go.mod but now they refer to modules "unrelated" with their project.

This requirement will stay for 3rd tier requirements and so on and so forth, every time further away from the module they are importing.

I propose to update the test module go.mod to require a real version of CAPI (v1.1.0). We should maintain the local folder replace directive for better CAPI developer experience.

This change is trivial (happy to submit a PR) and it shouldn't affect at all CAPI developers or build systems. However, in order to be consistent, we should keep updating the test module with new released versions of CAPI. Since this adds an extra step to the release process, and hence a sustained effort, I error on the side of being cautious and opened this issue to reach consensus.

This new step should ensure that before tagging a new CAPI version (vX.Y.Z), the test module go.mod is updated to reflect sigs.k8s.io/cluster-api vX.Y.Z. This shouldn't cause any issues, even if the tag vX.Y.Z doesn't exist at that time, because the replace sigs.k8s.io/cluster-api => ../ takes precedence.

I made this issue a feature request since it's no a bug per se, feel free to change it 🙂

/kind feature

[sbueringer]

Thx for filing this issue and the very clear description of the problem and a potential solution.

Just fyi, there is also that issue which targets the same problem: #5184 (if I see correctly)

Setting a real version in the test module sounds good to me in general, but I think the main problem is when do we update the version of CAPI in the test module and how we make sure it's up-to-date.

Let's assume we set the upcoming version right before every release:

• release 1.0 => test/go.mod: cluster-api sigs.k8s.io/cluster-api v1.0.0
• release 1.1 => test/go.mod: cluster-api sigs.k8s.io/cluster-api v1.1.0 (bumped from v1.0.0)
• ...

If we do this change on the main branch, the problem is that when someone is importing the test module between those releases they will get cluster-api v1.0.0 which is not the version that the test module on the main branch is based on. It could be that it doesn't even compile.

So I wonder if we only should do this on release-branches. I think this would come down to setting the version in the test go module right before the release. So all our releases would have the correct corresponding cluster-api version. The commits between releases on the release-branch would have a wrong version, but that's probably not too problematic, given that release branches are relatively stable.

I think we have to change the version right before the release, because the module with a non-existing CAPI version won't be importable for users either.

This would solve the issue for folks which are importing a CAPI release / release-branch (with a slightly wrong version), but of course it would still be the same issue if someone depends on main. Downside is also that this adds a manual step to our release process, but I think it's worth considering given that it makes it easier to import CAPI. We could add this to our release documentation.

I think on main I would prefer forcing folks to an explicit replace vs. giving them a version implicitly which is off by one minor version.

[fabriziopandini]

What about instead suggesting the providers have their own test module, so the import/redirect for the CAPI test module don't go up the chain of importers unless they intentionally pick it up

[fabriziopandini]

Note: the test module has been added to avoid test dependency like kind or docker to leak into the core CAPI dependencies

[vincepri]

/milestone Next

[g-gaston]

@sbueringer

So I wonder if we only should do this on release-branches. I think this would come down to setting the version in the test go module right before the release. So all our releases would have the correct corresponding cluster-api version. The commits between releases on the release-branch would have a wrong version, but that's probably not too problematic, given that release branches are relatively stable.

In my opinion this is ok. If someone is importing an specific commit from a release branch (I think this is quite unusual), I think is acceptable to ask them to deal with possible (although as you said, unlikely) breaking changes. They would just need to add a replace for the main module to the same commit they are already importing. Which is the same "extra work" that everyone has to currently do anyway.

I think we have to change the version right before the release, because the module with a non-existing CAPI version won't be importable for users either.
Totally agree.

I think on main I would prefer forcing folks to an explicit replace vs. giving them a version implicitly which is off by one minor version.
I think this is ok as well and probably even a good idea. We make importing capi easier for most folks (the ones importing release tags). And for those ones living on the bleeding edge out of main directly, we ask them to add a manual replace to ensure everything works correctly.

@fabriziopandini

What about instead suggesting the providers have their own test module, so the import/redirect for the CAPI test module don't go up the chain of importers unless they intentionally pick it up
That would solve it for most folks importing providers, totally. However, I feel that we might just be pushing the problem one level up the chain. Plus, even if we go with that advice, it will be up to each provider to do it or not.

I think it all boils down to how we define the test module. Is it internal to the CAPI repo and hence we don't intend other repos to import it? Or is it a tool we want to offer to providers or other capi related projects? If the answer is the latter, I believe we should find a way to make easily importable without "polluting" the golang ecosystem.

That said, I understand we need to find a balance between external dev experience and the burdens we add to this project to support that. I do think that what @sbueringer proposes (updating the require import version only on release branches) strikes a good balance. But I understand that's totally a personal opinion 🙂

[fabriziopandini]

I'm personally -1 to change modules on release branches, I see this leading to problems.

I think it all boils down to how we define the test module. Is it internal to the CAPI repo and hence we don't intend other repos to import it? Or is it a tool we want to offer to providers or other CAPI related projects?

Test module is designed for being consumed by providers, not so sure about the second-level import use case, but I get the point.

The best solution that comes to my mind is something similar to staging repositories in Kubernetes; a copy of CAPI test framework in another repo where we can publish a release without the redirect. The downside of this is all the automation required to make this happen.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[AverageMarcus]

/remove-lifecycle stale

I hit this issue today when including cluster-api-provider-aws as a dependency in my project. Would be good to have consensus on how this should be handled. (e.g. fix in CAPI vs. getting all providers to update their test structure)

[sbueringer]

The current situation is that consumers of our cluster-api/test module have to add a replace statement (e.g. replace sigs.k8s.io/cluster-api => sigs.k8s.io/cluster-api v1.1.2) to their go.mod file.

I'm not sure we found a good alternative yet that doesn't introduce too much toil.

I'm not sure what you mean with "getting all providers to update their test structure", adding the replace statement?
Or having a separate module for their test code so they don't have a dependency to cluster-api/test in their main module?
The latter might be good anyway so they don't leak all our test dependencies into their main go.mod file and into the go.mod files of their consumers.

That's btw the reason we have a separate test module. That consumers of Cluster API are not forced to also get our entire test dependencies transitively.

[AverageMarcus]

Or having a separate module for their test code so they don't have a dependency to cluster-api/test in their main module?

Yeah that's what I was thinking but it's quite a lot of work for all the providers. If that's going to be the recommended approach then the provider contract docs should also be updated to include it.

[sbueringer]

We can certainly discuss alternatives. I personally just don't have a good solution yet.

(a doc update makes sense, maybe more in the implementers guide, doesn't sound like a contract to me)

[AverageMarcus]

Implementers guide is what I meant, just couldn't remember the right name.

I also don't have a good solution. 😞

[fabriziopandini]

/triage accepted
/help

/remove-kind feature
/kind documentation
according to the latest comments about providing some guidelines

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help

/remove-kind feature
/kind documentation
according to the latest comments about providing some guidelines

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority backlog

[fabriziopandini]

The Cluster API project currently lacks enough contributors to adequately respond to all issues and PRs.

Folks kind of learned how to deal with the required replace by looking at existing providers (see. e.g. CAPV), but no one volunteered yet to document it.

If someone implementing a new provider find time to contribute back some learning, this will be welcome anytime
/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

The Cluster API project currently lacks enough contributors to adequately respond to all issues and PRs.

Folks kind of learned how to deal with the required replace by looking at existing providers (see. e.g. CAPV), but no one volunteered yet to document it.

If someone implementing a new provider find time to contribute back some learning, this will be welcome anytime
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.