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

generate: make CSV generator for Go GVKs package-aware #4445

Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Jan 28, 2021

Description of the change:

  • internal/generatel/clusterserviceversion/bases/definitions: make the owned CRD generator package- and type-aware so multiple packages containing the same type names can be used.

Motivation for the change: currently the CSV generator indexes types in the struct-field hierarchy by type name only, which is potentially not unique between packages. This PR makes the generator package-aware.

Closes #4409
Closes #3744

/kind bug

Signed-off-by: Eric Stroczynski ericstroczynski@gmail.com

Checklist

If the pull request includes user-facing changes, extra documentation is required:

owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 28, 2021
@estroz estroz added this to the v1.4.0 milestone Jan 28, 2021
@estroz
Copy link
Member Author

estroz commented Feb 1, 2021

/assign @jmrodri

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

I would've preferred to see unit tests directly for the ast.go file but seems to be covered by the larger definitions_test.

}

// findPackagePathForSelExpr returns the package path corresponding to the package name used in expr if it exists in im.
func (im importIdents) findPackagePathForSelExpr(expr *ast.SelectorExpr) (pkgPath string) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Not a fan of this method name but I don't have a better option :) so not blocking this PR.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 2, 2021
@estroz estroz merged commit 169778f into operator-framework:master Feb 2, 2021
@estroz estroz deleted the bugfix/package-scoped-csv-types branch February 2, 2021 20:50
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 4, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 4, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
@estroz
Copy link
Member Author

estroz commented Feb 4, 2021

/cherry-pick v1.3.x

@openshift-cherrypick-robot

@estroz: new pull request created: #4480

In response to this:

/cherry-pick v1.3.x

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.

reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
Signed-off-by: Rein van 't Veer <reinvantveer@gmail.com>
Signed-off-by: reinvantveer <rein.van.t.veer@geodan.nl>
rearl-scwx pushed a commit to rearl-scwx/operator-sdk that referenced this pull request Feb 5, 2021
…ework#4445)

internal/generatel/clusterserviceversion/bases/definitions: make the
owned CRD generator package- and type-aware so multiple packages
containing the same type names can be used.

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>Signed-off-by: rearl <rearl@secureworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
4 participants