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

internal/pkg/scaffold: add group.go scaffold to prevent deepcopy parse errors #1401

Merged
merged 6 commits into from
Jun 14, 2019

Conversation

estroz
Copy link
Member

@estroz estroz commented May 8, 2019

Description of the change: scaffold a pkg/apis/<group>/group.go file.

Motivation for the change: The following error is generated by the gengo parser:

$ operator-sdk generate openapi
W0507 16:56:26.785419   21221 parse.go:239] Ignoring child directory github.com/test-org/operator/pkg/apis/<group>: No files for pkg "github.com/test-org/operator/pkg/apis/<group>"

which doesn't look great. While we could skip reporting the error if it has that form, we might accidentally skip a valid error. Adding a package file to the group package corrects the error without any other changes.

Closes #1502, closes #1546

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 8, 2019
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Thanks for this!

Just curious would this have any effect on already existing operators when they add a new api? Otherwise lgtm!

@estroz
Copy link
Member Author

estroz commented May 8, 2019

@lilic @joelanford the SDK will scaffold pkg/apis/<group>/stub.go if one does not exist. I've also added a check for existence of any go files in pkg/apis/<group>; if one exists, stub.go is not scaffolded since it is unnecessary. PTAL.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Makes sense. Still LGTM.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying!! 👍

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

As discussed offline I think it's better if we don't add a new scaffold just to suppress an upstream warning.
This is a noticeable change in a user's project and the reason for it will not be immediately obvious.
Plus if the warning is suppressed later on upstream then we will have this scaffold for no reason.

We should first check if it's possible to address this upstream. And if not then probably suppress it in our CLI.
If we look out for this exact warning then hopefully there shouldn't be any unintended side effects of suppressing other future warnings.
Either way adding a new scaffold for this should be a last resort if at all necessary.

@estroz
Copy link
Member Author

estroz commented May 21, 2019

@hasbro17 this is actually an "issue" with go/build, and it looks like the intended behaviour is to error.

From one of the issue comments:

This issue appears to extend further - build.Import with build.FindOnly should return the directory path resolved for an import, even if there are no Go files in the directory, but this does not seem to be the case with Go modules.

This is exactly what k8s.io/gengo is doing, which is used by controller-tools, and by dependency us, to generate CRD's. If the intended behaviour is indeed to error if there are no go files in a package, by implication the correct way to structure packages is to always have at least one go file in them, right? So perhaps this is the best solution.

@lilic
Copy link
Member

lilic commented May 27, 2019

Coming back to this, while I was at the workshop a lot of users were super confused by the warning and came to me asking if this is a problem or not. So would be good to bypass this in a way, or suppress it just to avoid confusion IMHO.

Copy link
Contributor

@hasbro17 hasbro17 left a comment

Choose a reason for hiding this comment

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

LGTM after nit since there's no other way to suppress this and it's causing confusion for end users.

@estroz Also mention this in the CHANGELOG in the Added section that we now scaffold group.go so gengo warnings are suppressed.
It's a (minor) change visible to end users so it's best to point out why.

@estroz estroz changed the title internal/pkg/scaffold: add stub.go scaffold to prevent deepcopy parse errors internal/pkg/scaffold: add group.go scaffold to prevent deepcopy parse errors Jun 14, 2019
@estroz
Copy link
Member Author

estroz commented Jun 14, 2019

/test sanity

@estroz estroz merged commit 67927bf into operator-framework:master Jun 14, 2019
@estroz estroz deleted the scaffold-stub branch June 14, 2019 18:50
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 8, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 8, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
NicolasT added a commit to scality/metalk8s that referenced this pull request Jul 9, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 12, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
slaperche-scality pushed a commit to scality/metalk8s that referenced this pull request Jul 15, 2019
Without such file in place, code-generation using `operator-sdk generate
openapi` tends to fail. This is fixed in later versions of
`operator-sdk`, which generate exactly this file when scaffolding a new
API.

See: operator-framework/operator-sdk#1401
See: operator-framework/operator-sdk#1502
See: operator-framework/operator-sdk#1546
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning during add api about missing package cannot find module providing package
5 participants