-
Notifications
You must be signed in to change notification settings - Fork 699
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
update k8s dependency to stable version #230
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
Hi @wackxu. Thanks for your PR. I'm waiting for a kubernetes or tensorflow member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
Overall looks good; just one minor question about a new dependency. Reviewed 17 of 18 files at r1. glide.lock, line 78 at r1 (raw file):
Do you know why this dependency got added? Do we use it directly or is it a transitive dependency? Comments from Reviewable |
/ok-to-test |
@jlewi That is a transitive dependency and it is auto added when I run |
cmd/tf_operator/main.go
Outdated
"k8s.io/client-go/tools/record" | ||
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
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.
this PR generated LGTM, small nits should format imported package:
import (
"flag"
"fmt"
"os"
"os/signal"
"runtime"
"time"
"io/ioutil"
log "github.com/golang/glog"
"github.com/ghodss/yaml"
"github.com/tensorflow/k8s/pkg/controller"
"github.com/tensorflow/k8s/pkg/util"
"github.com/tensorflow/k8s/pkg/util/k8sutil"
"github.com/tensorflow/k8s/pkg/spec"
"github.com/tensorflow/k8s/version"
election "k8s.io/client-go/tools/leaderelection"
"k8s.io/client-go/tools/leaderelection/resourcelock"
"k8s.io/client-go/tools/record"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)
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.
@zjj2wry Thanks for your review, Done
#215 is working on refactor the TfJob to use K8s libraries, before we merge that, we should first to update k8s dependency to stable version.
@jlewi Any thoughts? can you take a look at this?
This change is