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

feat(dataSources): log runtime error when defaultData isn't provided #7591

Merged
merged 2 commits into from
Nov 5, 2019

Conversation

erikmunson
Copy link
Member

Since this is a new required field and might unexpectedly mess up someone's custom data source in a non-obvious way, let's log an error with some context to make the resolution more obvious.

I originally wanted to just throw a full-on Error, but those always get swallowed and end up making the application fail to load without showing up in the console.

Copy link
Contributor

@caseyhebebrand caseyhebebrand left a comment

Choose a reason for hiding this comment

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

LGTM

@erikmunson erikmunson merged commit 167a151 into spinnaker:master Nov 5, 2019
@erikmunson erikmunson deleted the data-source-default-error branch November 5, 2019 00:48
@christopherthielen
Copy link
Contributor

but those always get swallowed and end up making the application fail to load without showing up in the console.

What? Really? Do we know why this happens? I'd like this not to happen.

erikmunson pushed a commit to erikmunson/deck that referenced this pull request Nov 6, 2019
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
erikmunson pushed a commit that referenced this pull request Nov 6, 2019
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (#7585)
df7120a chore(help): Update help.contents.ts (#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (#7583)
8d478a0 fix(helptext): clarify text for stage timeout (#7336)
@erikmunson
Copy link
Member Author

@christopherthielen yeah it's been that way forever, and not just limited to data sources. Anything that throws during app setup/bootstrapping has its exception tossed out and the 'Application not found' thing comes up. Just took a few mins to build out a 'Something went wrong' state and log actual exceptions: #7599

Jammy-Louie pushed a commit to pivotal/deck that referenced this pull request Nov 8, 2019
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
sergiopena pushed a commit to sergiopena/deck that referenced this pull request Jan 10, 2020
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
yunzhangit pushed a commit to yunzhangit/deck that referenced this pull request Mar 28, 2021
5acb0d0 feat(managed): Add status popovers, new props for HoverablePopover
88b8e6c feat(managed): add resource status indicator to security groups
f02b70d feat(managed): add managed resource data to security group groups
14283b2 feat(managed): add resource status indicator to load balancers
aaa349b feat(managed): add managed resource data to load balancer groups
a6d7756 feat(managed): add resource status indicator to clusters
8453260 feat(managed): add managed resource data to cluster groups
167a151 feat(dataSources): add runtime error when defaultData isn't provided (spinnaker#7591)
a10e068 feat(core/presentation): Reduce unnecessary renders in useLatestPromise, add tests
90b249d feat(core/presentation): Reduce unnecessary renders in useDebouncedValue, add tests
7ff7aa1 refactor(core/presentation): extract useIsMountedRef hook
b18b577 chore(package): update to @types/enzyme@3.10.3, enzyme@3.10.0, enzyme-adapter-react-16@1.15.1 (spinnaker#7585)
df7120a chore(help): Update help.contents.ts (spinnaker#7588)
98e8a4b fix(rerun): Hiding re-run as strategies should not be re-run (spinnaker#7583)
8d478a0 fix(helptext): clarify text for stage timeout (spinnaker#7336)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants