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

Propagate error from NewREST #80952

Merged
merged 1 commit into from Aug 13, 2019

Conversation

@tedyu
Copy link
Contributor

commented Aug 3, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
This PR addresses TODO from pkg/registry/scheduling/priorityclass/storage/storage.go :

        if err := store.CompleteWithOptions(options); err != nil {
               panic(err) // TODO: Propagate error up
NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot requested review from liggitt and mwielgus Aug 3, 2019

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch 3 times, most recently from aed09fd to f0432ce Aug 3, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

/test pull-kubernetes-integration

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch 3 times, most recently from 9569341 to f68a9a4 Aug 3, 2019

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from f68a9a4 to e666d7b Aug 5, 2019

@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Aug 5, 2019

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from e666d7b to 5c27f1f Aug 5, 2019

@k8s-ci-robot k8s-ci-robot added size/M and removed size/S labels Aug 5, 2019

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from 5c27f1f to 2af5570 Aug 5, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

cc @piosz

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from 2af5570 to 2b33a95 Aug 6, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@liggitt
Please take another look.

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@oomichi
The error message you suggested is the same as the existing ones in the tests.
If you think the following would deliver more information for debugging, please let me know.

               t.Errorf("unexpected error from REST storage: %v", err)
@oomichi

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@tedyu

If you think the following would deliver more information for debugging, please let me know.
t.Errorf("unexpected error from REST storage: %v", err)

Thanks for your suggestion.
That seems a good message for debugging.

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from 9b4e104 to 793a1b1 Aug 9, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@oomichi
The change in error message has been done.

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@oomichi
Do you have other comments ?

thanks

@oomichi

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

Thanks for updating

/lgtm

@oomichi

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

/priority important-longterm

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from 793a1b1 to bab56bf Aug 9, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 9, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@liggitt
Can you take another look ?

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@liggitt
For commit bab56bf, I corrected more places where NewStorage used to panic.

pkg/registry/core/event/storage/storage_test.go Outdated Show resolved Hide resolved
pkg/registry/node/rest/runtime_class.go Show resolved Hide resolved
pkg/registry/node/rest/runtime_class.go Show resolved Hide resolved
test/integration/auth/rbac_test.go Outdated Show resolved Hide resolved
test/integration/auth/rbac_test.go Outdated Show resolved Hide resolved
test/integration/auth/rbac_test.go Outdated Show resolved Hide resolved
test/integration/auth/rbac_test.go Outdated Show resolved Hide resolved
apiGroupInfo, _ := certStorageProvider.NewRESTStorage(masterCfg.ExtraConfig.APIResourceConfigSource, masterCfg.GenericConfig.RESTOptionsGetter)
apiGroupInfo, _, err := certStorageProvider.NewRESTStorage(masterCfg.ExtraConfig.APIResourceConfigSource, masterCfg.GenericConfig.RESTOptionsGetter)
if err != nil {
t.FailNow()

This comment has been minimized.

Copy link
@liggitt

liggitt Aug 12, 2019

Member

8000+ instances of Fatal/Fatalf in our tests. 57 instances of FailNow(). Prefer Fatal/Fatalf

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from bab56bf to c99bd41 Aug 12, 2019

@tedyu tedyu force-pushed the tedyu:storage-ret-err branch from c99bd41 to 87b2a31 Aug 12, 2019

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

@liggitt
Your comments have been addressed - including Fatalf

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

/lgtm
/approve

great cleanup, thanks

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tedyu

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

@liggitt

This comment has been minimized.

Copy link
Member

commented Aug 12, 2019

/retest

@tedyu

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2019

/test pull-kubernetes-integration

@k8s-ci-robot k8s-ci-robot merged commit 890b50f into kubernetes:master Aug 13, 2019

23 checks passed

cla/linuxfoundation tedyu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 13, 2019

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.