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

Imports ordering #1187

Merged
merged 1 commit into from Jan 7, 2020
Merged

Imports ordering #1187

merged 1 commit into from Jan 7, 2020

Conversation

@Adirio
Copy link
Contributor

Adirio commented Nov 11, 2019

Imports have been grouped and sorted in the whole project. Two different schemas have been followed.

For kubebuilder source files, the imports have been split into three groups according to the current policy: standard library are placed first, followed by packages non related to k8s and at the end k8s packages.

import (
    std

    non-k8s

    k8s
)

Scaffolded files use the schema that go formatting tools (golang.org/x/tools/imports) implement: standard library first, followed by external packages and at the bottom imports from the current repository.

import (
    std

    external

    repository
)

Template strings for scaffolded files need to keep imports in a single group and imports.Process wil handle the divission.

Linter message:

Unsorted imports inspection

Linter description:

Reports unsorted imports.

References:

Note: despite being the de facto standard, couldn't find a CodeReviewComments or Effective Go section that talks about the order of imports in each group, but you can see in the example how this rule is being followed.

Closes: #1183

/kind cleanup

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 11, 2019

Hi @Adirio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@Adirio Adirio force-pushed the Adirio:import-order branch from 0bc521d to 5215c05 Nov 12, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 12, 2019

This was already rebased, any idea why Prow is detecting a rebase is needed?

@camilamacedo86

This comment has been minimized.

Copy link
Member

camilamacedo86 commented Nov 12, 2019

/assign @pmorie

@camilamacedo86

This comment has been minimized.

Copy link
Member

camilamacedo86 commented Nov 12, 2019

/assign @camilamacedo86

Copy link
Member

camilamacedo86 left a comment

Hi @pmorie wdyt?
For me it is very small so it is ok.
/lgtm /approve

@Adirio Adirio changed the title Imports ordering [WIP] Imports ordering Nov 13, 2019
@Adirio Adirio force-pushed the Adirio:import-order branch from 5215c05 to bfb2779 Nov 13, 2019
@Adirio Adirio changed the title [WIP] Imports ordering Imports ordering Nov 13, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 13, 2019

The suggested groups and orders have been created automatically by selecting the following options:

  • Use back quotes for imports
  • Add parentheses for a single import
  • Sorting type: gofmt
  • Group stdlib imports
    • Move all stdlib imports in a single group
  • Group current project imports
  • Move all imports in a single declaration
@Adirio Adirio force-pushed the Adirio:import-order branch from bfb2779 to 02f9582 Nov 15, 2019
@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Nov 15, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 18, 2019

Travis is failing because the testdata files include blank lines and the generated ones don't. However, the scaffolding template does have those blank lines. I am not sure what is removing those blank lines, import.Process is handling other import groups (aka. blank lines) properly but it is not handling those ones accordingly.

@mengqiy @DirectXMan12

Copy link
Member

camilamacedo86 left a comment

When I approved it as not with these changes.
So, I would like to dismiss my review until I am able to check it again.

@Adirio Adirio force-pushed the Adirio:import-order branch from aa18475 to b30633f Nov 21, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 21, 2019

/hold

I'm trying to fix some issues, will cancel the hold when it is ready so that reviewers do not need to follow the fix process.

@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 21, 2019

Current Travis issues are on the namespace-related scaffold because it doesn't belong to the local project but to a builtin api.
I think that they can be solved by not scaffolding different import groups and letting imports.Process do it for us. For that we first need to set imports.LocalPrefix to the local repo (which should be obtainable from the input.Input easily).

@Adirio Adirio force-pushed the Adirio:import-order branch from 1f076ee to 8dc8a9b Nov 21, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 21, 2019

@Adirio: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubebuilder-e2e aa18475 link /test pull-kubebuilder-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"

corev1 "k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@Adirio

Adirio Nov 21, 2019

Author Contributor

This is a workaround as I have been unable to make it join the previous group.

The reason is simple to understand, but not easy to solve. When kubebuilder scaffolds this file, all the imports are in a single group, and imports.Process is able to create the 3 separate groups (std/external/internal) correctly. But some imports are added aftwerwards through the // +kubebuilder:scaffold:imports tag. When they are added, some groups are already created, and imports.Process will not mix the groups together except for the standard library imports that are always placed at the top. So despite corev1 "k8s.io/api/core/v1" being a external import, it is placed in a separate group. I think that the easiest fix will be making file updates first remove every blank line in the imports section, thus eliminating the previously created groups, and letting import.Process recreate the groups. The problem of this approach is that groups manually created by users will be removed.

This comment has been minimized.

Copy link
@DirectXMan12

DirectXMan12 Dec 5, 2019

Contributor

The problem of this approach is that groups manually created by users will be removed.

That's fine, I think.

Do you want to do that here, or in a follow up?

This comment has been minimized.

Copy link
@Adirio

Adirio Dec 6, 2019

Author Contributor

I would say in a follow up as that would require messing with templates manually.

testdata/project-v2/main.go Show resolved Hide resolved
@camilamacedo86 camilamacedo86 removed their assignment Nov 21, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 21, 2019

/hold cancel

@mengqiy I think this PR is ready to get merged if @justinsb is ok as some of the changes are motivated by #951

The only gotcha to be aware is the non-resolved conversation above. An import in the scaffolded main.go and controllers/suite_test.go is not optimally placed (while still technichaly correct).

@Adirio Adirio force-pushed the Adirio:import-order branch from 8dc8a9b to 02b8f79 Nov 26, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Nov 26, 2019

Rebased (no changes)

Copy link
Contributor

DirectXMan12 left a comment

single comment inline, otherwise LGTM

"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"

corev1 "k8s.io/api/core/v1"

This comment has been minimized.

Copy link
@DirectXMan12

DirectXMan12 Dec 5, 2019

Contributor

The problem of this approach is that groups manually created by users will be removed.

That's fine, I think.

Do you want to do that here, or in a follow up?

@Adirio Adirio force-pushed the Adirio:import-order branch from 02b8f79 to d6f9bd1 Dec 9, 2019
@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Dec 9, 2019

Rebased and working, can be merged

@Adirio

This comment has been minimized.

Copy link
Contributor Author

Adirio commented Dec 10, 2019

@DirectXMan12 Could we merge this?

@Adirio Adirio force-pushed the Adirio:import-order branch from d6f9bd1 to 4d2f34f Dec 16, 2019
Kubebuilder files are grouped by std/non-k8s/k8s while scaffolded files are grouped by std/external/internal
Signed-off-by: Adrian Orive <adrian.orive.oneca@gmail.com>
@Adirio Adirio force-pushed the Adirio:import-order branch from 4d2f34f to 4e7ec4b Jan 7, 2020
@Adirio Adirio mentioned this pull request Jan 7, 2020
@DirectXMan12

This comment has been minimized.

Copy link
Contributor

DirectXMan12 commented Jan 7, 2020

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 7, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Adirio, DirectXMan12

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 86dc861 into kubernetes-sigs:master Jan 7, 2020
7 of 8 checks passed
7 of 8 checks passed
tide Not mergeable.
Details
cla/linuxfoundation Adirio authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/kubebuilder/deploy-preview Deploy preview ready!
Details
pull-kubebuilder-e2e-k8s-1-14-1 Job succeeded.
Details
pull-kubebuilder-e2e-k8s-1-15-3 Job succeeded.
Details
pull-kubebuilder-e2e-k8s-1-16-2 Job succeeded.
Details
pull-kubebuilder-test Job succeeded.
Details
@Adirio Adirio deleted the Adirio:import-order branch Jan 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.