-
Notifications
You must be signed in to change notification settings - Fork 529
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: font 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall it LGTM
"github.com/kubernetes-sigs/federation-v2/pkg/controller/util" | ||
"k8s.io/klog" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
k8s.io is a third party package. It should be better on top of federation package as indepedent section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. This is a default gofmt
placement since there's no space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention we've converged on is:
import (
<system imports (e.g. fmt)>
<3rd party imports (e.g. glog, errors)>
<k8s imports (e.g. klog, metav1)>
<fedv2 imports>
[<test-specific imports>]
)
@marun I made a comment about it that it looks like it is WIP and hasn't been updated recently. It's not my intent to take the work from @poothia (apologies!), it was more to see it through since a follow-up PR for the webhook framework which depends on it is forthcoming. |
#695 is WIP, but this switch to use
klog
is needed in order to enable webhook dependencies in a follow-up PR.Posting on behalf of @pmorie who did the work.
Fixes #694