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

improve kubectl error message when an object with no kind is passed to the resource builder #9357

Merged
merged 1 commit into from Jun 8, 2015

Conversation

@mikedanese
Copy link
Member

@mikedanese mikedanese commented Jun 6, 2015

Kindless api objects have their error message printed instead of glogged.

Fixes #9010

➜  kubernetes git:(kubectl-kind-err) ✗ cat bonkers/no-kind.yml 
apiVersion: v1beta3
metadata:
  labels:
    name: redis
    redis-sentinel: "true"
    role: master
  name: redis-master
spec:
  containers:
    - name: master
      image: kubernetes/redis:v1
      env:
        - name: MASTER
          value: "true"
      ports:
        - containerPort: 6379
      resources:
        limits:
          cpu: "1"
      volumeMounts:
        - mountPath: /redis-master-data
          name: data
    - name: sentinel
      image: kubernetes/redis:v1
      env:
        - name: SENTINEL
          value: "true"
      ports:
        - containerPort: 26379
  volumes:
    - name: data
      emptyDir: {}
➜  kubernetes git:(kubectl-kind-err) ✗ kubectl create -f bonkers/no-kind.yml
error: unable to load file "bonkers/no-kind.yml": kind not set in "bonkers/no-kind.yml"
error: no objects passed to create

Alternatively, we could not ContinueOnError by default. Any feelings @jlowdermilk @thockin?

@j3ffml
Copy link
Contributor

@j3ffml j3ffml commented Jun 6, 2015

It would make sense for the resource visitor to not ContinueOnError by default if there's only one object. @smarterclayton for input on that thought.

This change lgtm, though. It's a simple and definite improvement over the existing behavior.

@mikedanese mikedanese changed the title Kubectl kind err improve kubectl error message when an object with no kind is passed to the resource builder Jun 6, 2015
@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 6, 2015

@jlowdermilk might make sense. We probably need to queue an issue to look at side loaded errors in visitor and see if there's an alternative to the way we have to aggressively continue loops even when errors happen (because someone might want to continueOnError). It might be a change to visitor to allow it to accept an error, like Go's path walker.

For now better to just report the message. Can we add a TODO in there though to revisit?

@@ -220,7 +220,7 @@ func (v *PathVisitor) Visit(fn VisitorFunc) error {
if !v.IgnoreErrors {
return err
}
glog.V(2).Infof("Unable to load file %q: %v", v.Path, err)
fmt.Printf("error: unable to load file %q: %v\n", v.Path, err)
Copy link
Contributor

@smarterclayton smarterclayton Jun 6, 2015

Choose a reason for hiding this comment

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

Should be to os.Stderr

@bgrant0607
Copy link
Member

@bgrant0607 bgrant0607 commented Jun 6, 2015

cc @ghodss

@mikedanese mikedanese force-pushed the kubectl-kind-err branch from 59ebe21 to 7b7d8cc Jun 6, 2015
@mikedanese
Copy link
Member Author

@mikedanese mikedanese commented Jun 6, 2015

@smarterclayton
Copy link
Contributor

@smarterclayton smarterclayton commented Jun 8, 2015

LGTM

@mikedanese
Copy link
Member Author

@mikedanese mikedanese commented Jun 8, 2015

Created #9405 as a spin off issue for the discussed error handling refactoring.

krousey added a commit that referenced this issue Jun 8, 2015
improve kubectl error message when an object with no kind is passed to the resource builder
@krousey krousey merged commit 8ce921e into kubernetes:master Jun 8, 2015
3 checks passed
@mikedanese mikedanese deleted the kubectl-kind-err branch Jun 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants