Firehose: honor `optional` prop for watches on individual resources #1711

mareklibra · 2019-06-13T09:26:32Z

Introduce ignoreIfMissing Firehose resource property.

If set to true, a resource is skipped from generic
loadError 404 checking (cross-resources). So the overall data
retrieval does not fail if a specified resource is missing.

Can be used to optionally query a resource but enable rendering
of children if it is missing (HTTP 404).

The new property is an addition to existing optional property which
does not block rendering of children if list-retrieval is still in progress.

With default value ignoreIfMissing = false, the component is
backward compatible.

If set to `true`, a resource is skipped from generic `loadError` 404 checking (cross-resources). So the overall data retrieval does not fail if a specified resource is missing. Can be used to optionally query a resource but enable rendering of children if it is missing (HTTP 404). The new property is an addition to existing `optional` property which does not block rendering of children if list-retrieval is still in progress. With default value `ignoreIfMissing = false`, the component is backward compatible.

openshift-ci-robot · 2019-06-13T09:26:46Z

Hi @mareklibra. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

atiratree · 2019-06-13T11:02:26Z

frontend/public/components/utils/firehose.jsx

+      stuff = stuff.toJS();
+      stuff.ignoreIfMissing = props.ignoreIfMissing;
+    }
+    return stuff || {};


can we use the same approach which is used for lists?

const data = k8s.getIn([reduxID, 'data']); return { data: data && data.toJS(), loadError: k8s.getIn([reduxID, 'loadError']), loaded: k8s.getIn([reduxID, 'loaded']), optional: props.optional, ignoreIfMissing: props.ignoreIfMissing, };

This way we can optionally swap the toJS with only immutables once they are needed in the future.

Actually the stuff is not the response received from API but already a wrapper around it, please see

console/frontend/public/reducers/k8s.ts

Line 135 in 98211bb

return state.mergeIn([id], {

I know, but it will be easier to extend for adding a support of raw immutables

meaning that toJS may not be used at all

If this is needed, I propose to do it in a follow-up.
It would require refactoring of code around the reducer and this is unrelated to original need behind this PR.

spadgett · 2019-06-13T11:22:45Z

@mareklibra I'm confused: how is this different than optional? Can you give a specific example where optional doesn't work?

mareklibra · 2019-06-13T11:50:32Z

frontend/public/components/utils/firehose.jsx

@@ -208,6 +223,7 @@ Firehose.propTypes = {
    selector: PropTypes.object,
    fieldSelector: PropTypes.string,
    isList: PropTypes.bool,
-    optional: PropTypes.bool,
+    optional: PropTypes.bool, // do not block children-rendering while resource is still being loaded
+    ignoreIfMissing: PropTypes.bool, // do not fail if the resource is missing (HTTP 404)


Someone can opinionate that optional can be reused for that feature, as the difference is quite small.
I prefer to keep semantics of optional as it is, so existing functionality will not be broken.

If not clear otherwise: if optional=true, the children are rendered no matter if loading is still in progress. And eventually causes error 404 afterwards.

With ignoreIfMissing=true only, the rendering is postponed till data are loaded or errored. But 404 is not a blocker for rendering. A child component can still check the error code, but the overall error checking is not affected.

It makes sense to set these props individually same as both together.

Having two similarly-named properties that behave almost the same, but subtly different, makes for a confusing API. I'm still unclear on the difference even after the explaination :) Can't callers check the loaded value if they don't want to render when loading?

I'd prefer either:

Using optional as-is

Changing optional if we think there is better behavior (as long as we can do so without breaking current use)

I only see 5 places where optional is used today, so changing behavior is an option as long we can test / update those places.

Again, I'm still unclear on what's different. Can you give a specific example?

I found that when reusing StatusBox component.

If optional=true for a single (non-list) object and 404 occurs, the Firehose injects top-level loadError with 404.

With the change proposed in this PR, the ignoreIfMissing resource will not affect the top-level loadError with 404.

My component structure is like:

<Firehose> <StatusBox> <MyComponent>

I can change behaviour in StatusBox by ignoring the top-level loadError and checking loadError on every loaded resource, but this is more complicated and would introduce more confusion.
Or I can re-implement StatusBox, but this goes against reusability.

Again, expected behaviour by this PR is:

optional=true ==> do not postpone rendering but fail on 404

ignoreIfMissing=true ==> postpone rendering but ignore 404

both set to true ==> do not block rendering and do not fail if 404 - the resource is really optional we do not care about it's availability.

If we change semantics of optional to act like the third option above only, it will be difficult to implement scenario for immediate (non-blocking) rendering of a page/detail and failing asynchronously if missing.
My PRs do not need this feature but I am not sure whether this is not already expected on other places.

If optional=true for a single (non-list) object and 404 occurs, the Firehose injects top-level loadError with 404.

That sounds like a bug. Are we sure that's how optional is intended to behave? Can we simply fix optional? :)

Such a "fix" would be simple.

I don't see optional defined anywhere except just the code.
It was my natural understanding that optional should ignore 404. But I am not sure whether something is not already relying on recent semantics.

I don't know the history, but I believe it was originally added to avoid 403 errors when you don't have permission. I don't see a reason to handle 404 the same way, though.

The places to check are:

frontend/public/components/resource-quota.jsx

frontend/public/components/operator-lifecycle-manager/k8s-resource.tsx

frontend/public/components/RBAC/role.jsx

frontend/public/components/RBAC/bindings.jsx

frontend/public/components/cluster-settings/cluster-settings.tsx

I don't see any other usages.

If a resource is `optional: true` then it is excluded from setting of top-level loadError. Firehose's child component can still check loadError by inspecting the resource directly.

mareklibra · 2019-06-13T14:36:24Z

@spadgett , I changed semantics of optional based on discussion and so could remove ignoreIfMissing.
Can you please have a look?

spadgett · 2019-06-13T15:13:56Z

/ok-to-test

spadgett

/lgtm

spadgett · 2019-06-13T17:44:50Z

frontend/public/components/utils/firehose.jsx

@@ -25,8 +25,12 @@ const processReduxId = ({k8s}, props) => {
  }

  if (!isList) {


Ah, OK. I see now. We didn't have proper support for optional with individual resources, only lists.

openshift-ci-robot · 2019-06-13T18:01:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mareklibra, spadgett

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:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2019-06-13T19:37:32Z

/retest

openshift-bot · 2019-06-13T23:14:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

…penshift#1711) * Firehose: introduce "ignoreIfMissing" resource property If set to `true`, a resource is skipped from generic `loadError` 404 checking (cross-resources). So the overall data retrieval does not fail if a specified resource is missing. Can be used to optionally query a resource but enable rendering of children if it is missing (HTTP 404). The new property is an addition to existing `optional` property which does not block rendering of children if list-retrieval is still in progress. With default value `ignoreIfMissing = false`, the component is backward compatible. * Firehose: fix the `optional` property for 404 If a resource is `optional: true` then it is excluded from setting of top-level loadError. Firehose's child component can still check loadError by inspecting the resource directly.

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 13, 2019

openshift-ci-robot requested review from dtaylor113 and kyoto June 13, 2019 09:26

mareklibra mentioned this pull request Jun 13, 2019

Kubevirt VmDetails #1682

Merged

4 tasks

atiratree reviewed Jun 13, 2019

View reviewed changes

mareklibra commented Jun 13, 2019

View reviewed changes

Firehose: fix the optional property for 404

b1e3578

If a resource is `optional: true` then it is excluded from setting of top-level loadError. Firehose's child component can still check loadError by inspecting the resource directly.

openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 13, 2019

spadgett added this to the v4.2 milestone Jun 13, 2019

spadgett added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jun 13, 2019

spadgett approved these changes Jun 13, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jun 13, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2019

spadgett changed the title ~~Firehose: introduce "ignoreIfMissing" resource property~~ Firehose: honor optional prop for watches on individual resources Jun 13, 2019

openshift-merge-robot merged commit 4653e3a into openshift:master Jun 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firehose: honor `optional` prop for watches on individual resources #1711

Firehose: honor `optional` prop for watches on individual resources #1711

mareklibra commented Jun 13, 2019 •

edited

Loading

openshift-ci-robot commented Jun 13, 2019

atiratree Jun 13, 2019 •

edited

Loading

mareklibra Jun 13, 2019 •

edited

Loading

atiratree Jun 13, 2019

atiratree Jun 13, 2019

mareklibra Jun 13, 2019

spadgett commented Jun 13, 2019

mareklibra Jun 13, 2019

spadgett Jun 13, 2019

spadgett Jun 13, 2019

mareklibra Jun 13, 2019

spadgett Jun 13, 2019

mareklibra Jun 13, 2019

spadgett Jun 13, 2019

mareklibra commented Jun 13, 2019

spadgett commented Jun 13, 2019

spadgett left a comment

spadgett Jun 13, 2019

openshift-ci-robot commented Jun 13, 2019

spadgett commented Jun 13, 2019

openshift-bot commented Jun 13, 2019

		@@ -25,8 +25,12 @@ const processReduxId = ({k8s}, props) => {
		}

		if (!isList) {

Firehose: honor optional prop for watches on individual resources #1711

Firehose: honor optional prop for watches on individual resources #1711

Conversation

mareklibra commented Jun 13, 2019 • edited Loading

openshift-ci-robot commented Jun 13, 2019

atiratree Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

mareklibra Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jun 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mareklibra commented Jun 13, 2019

spadgett commented Jun 13, 2019

spadgett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 13, 2019

spadgett commented Jun 13, 2019

openshift-bot commented Jun 13, 2019

Firehose: honor `optional` prop for watches on individual resources #1711

Firehose: honor `optional` prop for watches on individual resources #1711

mareklibra commented Jun 13, 2019 •

edited

Loading

atiratree Jun 13, 2019 •

edited

Loading

mareklibra Jun 13, 2019 •

edited

Loading