Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Cross-module support for filesystem paths #663

Merged
merged 1 commit into from
May 3, 2022

Conversation

akutz
Copy link
Contributor

@akutz akutz commented Apr 23, 2022

This patch updates the controller-gen package loader to support loading packages across Go module boundaries.

This fixes #516 and enables vmware-tanzu/tanzu-framework#2222. Without this PR the CRDs generated from the referenced PR would be missing the types from that project's ./apis/run/v1alpha3 directory as it is a distinct Go module, whereas its siblings, v1alpha1 and v1alpha2 belong to the root module (please see the aforementioned PR as to why this is).

This patch has been tested by building the branch from vmware-tanzu/tanzu-framework#2222.

This patch updates the controller-gen package loader to support loading packages across Go module boundaries for filesystem paths. What does this actually mean? Imagine a project with the following structure:

my-project/
|
|-- go.mod
|-- go.sum
|
|-- apis/
    |
    |-- v1alpha1/
        |
        |-- go.mod
        |-- go.sum

Prior to this patch, the following command, executed from the root of my-project, would not include the package my-project/apis/v1alpha1:

controller-gen crd \
  paths=./apis/... \
  crd:trivialVersions=true \
  output:crd:dir=./config/crd/bases

The reason the above command would not parse the types from the package my-project/apis/v1alpha1 is because it is a distinct Go module from the root module, where the command was executed. In fact, while the above command would silently fail to include those types, the following command will fail with an error:

controller-gen crd \
  paths=./apis/v1alpha1 \
  crd:trivialVersions=true \
  output:crd:dir=./config/crd/bases

The above command fails because the underlying logic to load packages cannot load a package by filesystem path from one module when executed from the working directory of another module.

With this patch, it is now possible for the first command to successfully traverse Go module boundaries for roots that are filesystem paths ending with ... with the following, high-level logic:

  1. If no roots are provided then load the working directory and return early.

  2. Otherwise sort the provided roots into two, distinct buckets:

    1. package/module names
    2. filesystem paths

    A filesystem path is distinguished from a Go package/module name by the same rules as followed by the "go" command. At a high level, a root is a filesystem path IFF it meets ANY of the following criteria:

    • is absolute
    • begins with .
    • begins with ..

    For more information please refer to the output of the command go help packages.

  3. Load the package/module roots as a single call to packages.Load. If there are no filesystem path roots then return early.

  4. For filesystem path roots ending with ..., check to see if its descendants include any nested, Go modules. If so, add the directory that contains the nested Go module to the filesystem path roots.

  5. Load the filesystem path roots and return the load packages for the package/module roots AND the filesystem path roots.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 23, 2022
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 23, 2022
@akutz akutz force-pushed the feature/xmod branch 2 times, most recently from 1e6fd8b to e20367f Compare April 23, 2022 18:50
@akutz
Copy link
Contributor Author

akutz commented Apr 25, 2022

/assign @alvaroaleman

@akutz
Copy link
Contributor Author

akutz commented Apr 25, 2022

I force pushed a few updates to documentation to make the code easier to read.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2022
@akutz
Copy link
Contributor Author

akutz commented Apr 25, 2022

I just repushed to add some unit tests to pkg/loader.LoadRoots to ensure nothing unintended is being introduced.

@akutz
Copy link
Contributor Author

akutz commented Apr 25, 2022

I added additional assertions to the tests.

@lubronzhan
Copy link

Hi @droot @alvaroaleman @pwittrock could you help take a look? This is very useful to unblock us.

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple if nits. If at all possible I'd like to avoid a new "I really really want to recurse, even across module boundaries" notation and stick with the existing one for that

pkg/loader/loader.go Outdated Show resolved Hide resolved
pkg/loader/loader.go Outdated Show resolved Hide resolved
pkg/loader/loader.go Outdated Show resolved Hide resolved
pkg/loader/loader.go Outdated Show resolved Hide resolved
pkg/loader/testmods/childDirB/go.mod Outdated Show resolved Hide resolved
@akutz
Copy link
Contributor Author

akutz commented Apr 29, 2022

/hold

I realized this PR has a bug. Well, the bug is already present, but since this PR claims to add support for cross-module discovery, I feel this PR cannot claim to do so and not address this.

Basically if there's a project with a root Go module and controller-gen paths=./submod is used, where ./submod is a sub-module, then this would fail because the loader will return an error that the path./submod is not a member of the root module.

That is obviously not desirable. Let me think on how to address this.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 29, 2022
@akutz akutz force-pushed the feature/xmod branch 5 times, most recently from 904f3ff to 174eb42 Compare May 2, 2022 16:00
@akutz akutz force-pushed the feature/xmod branch 4 times, most recently from 05161c9 to 45c5cce Compare May 2, 2022 23:09
@akutz
Copy link
Contributor Author

akutz commented May 2, 2022

Okay, now the prior behavior (support no paths, support package/module names directly, basically anything you can do with go list) is now supported as well.

The one thing not yet supported is cross-module support when explicitly specifying a package name ending with /.... Cross-module support only works for local filesystems.

Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2022
@akutz
Copy link
Contributor Author

akutz commented May 3, 2022

/hold

I'm going to update it with a slight optimization.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2022
@akutz
Copy link
Contributor Author

akutz commented May 3, 2022

/hold cancel

Okay, removing the hold, and I will update the PR description as well. I added a bunch of documentation and some optimizations.

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 3, 2022
@akutz akutz changed the title ✨ Cross-module support for paths ✨ Cross-module support for filesystem paths May 3, 2022
This patch updates the controller-gen package loader to support
loading packages across Go module boundaries for filesystem paths.

What does this actually mean? Imagine a project with the following
structure:

my-project/
|
|-- go.mod
|-- go.sum
|
|-- apis/
    |
    |-- v1alpha1/
        |
        |-- go.mod
        |-- go.sum

Prior to this patch, the following command, executed from the root of
"my-project," would *not* include the package my-project/apis/v1alpha1:

    controller-gen crd \
      paths=./apis/... \
      crd:trivialVersions=true \
      output:crd:dir=./config/crd/bases

The reason the above command would not parse the types from the package
my-project/apis/v1alpha1 is because it is a distinct Go module from the
root module, where the command was executed. In fact, while the above
command would silently fail to include those types, the following
command will fail with an error:

    controller-gen crd \
      paths=./apis/v1alpha1 \
      crd:trivialVersions=true \
      output:crd:dir=./config/crd/bases

The above command fails because the underlying logic to load packages
cannot load a path from one package when executed from the working
directory of another package.

With this patch, it is now possible for the first command to
successfully traverse Go module boundaries for roots that are file-
system paths ending with "..." with the following, high-level logic:

    1. If no roots are provided then load the working directory and
       return early.

    2. Otherwise sort the provided roots into two, distinct buckets:

           a. package/module names
           b. filesystem paths

       A filesystem path is distinguished from a Go package/module name
       by the same rules as followed by the "go" command. At a high
       level, a root is a filesystem path IFF it meets ANY of the
       following criteria:

           * is absolute
           * begins with .
           * begins with ..

       For more information please refer to the output of the command
       "go help packages".

    3. Load the package/module roots as a single call to packages.Load.
       If there are no filesystem path roots then return early.

    4. For filesystem path roots ending with "...", check to see if its
       descendants include any nested, Go modules. If so, add the
       directory that contains the nested Go module to the filesystem
       path roots.

    5. Load the filesystem path roots and return the load packages for
       the package/module roots AND the filesystem path roots.
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, alvaroaleman

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit ae78097 into kubernetes-sigs:master May 3, 2022
@akutz akutz deleted the feature/xmod branch May 3, 2022 16:42
@sbueringer
Copy link
Member

For visibility, looks like this PR introduced this issue: #680

randomvariable added a commit to randomvariable/tanzu-framework that referenced this pull request Oct 12, 2022
due to kubernetes-sigs/controller-tools#663

Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
randomvariable added a commit to randomvariable/tanzu-framework that referenced this pull request Oct 12, 2022
due to kubernetes-sigs/controller-tools#663

Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
randomvariable added a commit to vmware-tanzu/tanzu-framework that referenced this pull request Oct 12, 2022
due to kubernetes-sigs/controller-tools#663

Signed-off-by: Naadir Jeewa <jeewan@vmware.com>
@mmingdai
Copy link

For visibility, this PR also introduced #837

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

controller-gen crd generation does not work across modules layouts
7 participants