Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error #5539

karthikjeeyar · 2020-05-22T12:51:45Z

Fixes: https://issues.redhat.com/browse/ODC-3725

Problem:

Visiting the pipeline run logs page, the logs component shows the error due to the string method is applied on a object.

Solution:
Add additional checks to make sure the string method is used on a string and if it is not a string added a empty array as a fallback.

Screenshots:

openshift-ci-robot · 2020-05-22T12:51:52Z

@karthikjeeyar: This pull request references Bugzilla bug 1839080, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error

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.

karthikjeeyar · 2020-05-22T12:52:25Z

/kind bug
/assign @andrewballantyne

andrewballantyne · 2020-05-22T16:50:09Z

frontend/packages/dev-console/src/components/pipelineruns/logs/Logs.tsx

@@ -35,7 +35,8 @@ const Logs: React.FC<LogsProps> = ({
  const appendMessage = React.useRef<(blockContent) => void>();
  appendMessage.current = React.useCallback(
    (blockContent) => {
-      const contentLines = blockContent.split('\n').filter((line) => !!line);
+      const contentLines =
+        typeof blockContent === 'string' ? blockContent.split('\n').filter((line) => !!line) : [];


I'll be honest... this is definitely a short term solution to get the error not to happen... but I wonder if it's not worth correcting it at the source.

Either way, I think the solution should be to exit early.

(blockContent) => { if (typeof blockContent !== 'string') return; //... }

I think we'd want to type blockContent as a string. Thus making our typeof check not a valid check maybe a check one step higher 🤔 .

I think the solution should be to go to co-fetch.js and handle this situation:

if (response.headers.get('Content-Length') === '0') { return Promise.resolve({}); }

Or maybe even better by updating the coFetchText:

export const coFetchText = (url, options = {}, timeout) => { return coFetchCommon(url, 'GET', options, timeout).then(() => { /* prevent non strings */ }); };

Handled non strings in CoFetchText, so that the consumer of this method will always get string response.

andrewballantyne

I like the solution, I think we can get this one step further though.

andrewballantyne · 2020-05-25T13:31:29Z

frontend/public/co-fetch.js

+    if (_.isObject(response)) {
+      return _.isEmpty(response) ? '' : JSON.stringify(response);
+    }
+    return response;


Suggested change

return response;

return _.toString(response);

Let's enforce this a bit more. null => '', false => 'false'. I think this is a good safety to have.

andrewballantyne · 2020-05-25T13:31:38Z

frontend/public/co-fetch.js

@@ -164,7 +164,12 @@ export const coFetchJSON = (url, method = 'GET', options = {}, timeout) => {
 };

 export const coFetchText = (url, options = {}, timeout) => {
-  return coFetchCommon(url, 'GET', options, timeout);
+  return coFetchCommon(url, 'GET', options, timeout).then((response) => {


Suggested change

return coFetchCommon(url, 'GET', options, timeout).then((response) => {

return coFetchCommon(url, 'GET', options, timeout).then((response): string => {

@andrewballantyne I dint change this because this is a co-fetch.js File, i can change it to ts file but all other methods are not typed here. WDYT?

Oh hahaha, sorry... on autopilot. Forget we have JS files. Nvm, you are good, ignore this change.

andrewballantyne

/lgtm

/assign @christianvogt

Need Christian to weigh in here as this is in the public folder.

christianvogt · 2020-05-26T13:09:22Z

frontend/public/co-fetch.js

-  return coFetchCommon(url, 'GET', options, timeout);
+  return coFetchCommon(url, 'GET', options, timeout).then((response) => {
+    if (_.isObject(response)) {
+      return _.isEmpty(response) ? '' : JSON.stringify(response);


This is odd because we're undoing what coFetchCommon has provided.
A better solution might be to split coFetchCommon into 2 functions such that one simply returns the response without calling .text() or .json() such that we use it here to return the text.

The problem with coFetchCommon method is, when the response.headers.get('Content-Length') === '0' it always resolves an {} empty object, by conditionally resolving an empty string or an empty object based on response.headers.get('Content-Type') , we can avoid the undoing logic altogether. WDYT?

andrewballantyne

This is a better solution. Was worried this was not going to be possible when I originally logged the ticket as we can't send the accepts request header for every API... but testing the response on the return might work. I'll need to do a quick test to verify this works as expected. Pretty sure the content reads text/plain when no content is there, but I am unsure if that's 100% true all the time since zero content doesn't really need a type.

sahil143

@karthikjeeyar tkn logs omit the step for which there are no logs and we try to show same logs as tkn logs. Not sure if we should do the same?

cc: @andrewballantyne

andrewballantyne · 2020-05-27T14:41:38Z

As discussed with @sahil143 - this is something we can move to 4.6. We're locking things down for 4.5 and let us just deal with the problems at hand and improve the slightly undesirables later.

sahil143 · 2020-05-27T14:45:24Z

story to track the above issue https://issues.redhat.com/browse/ODC-4002

andrewballantyne · 2020-05-27T19:16:27Z

/lgtm

christianvogt · 2020-05-27T22:58:52Z

/approve

openshift-ci-robot · 2020-05-27T22:59:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, karthikjeeyar

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~~ [christianvogt]

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

openshift-ci-robot · 2020-05-28T01:07:29Z

@karthikjeeyar: All pull requests linked via external trackers have merged: openshift/console#5539. Bugzilla bug 1839080 has been moved to the MODIFIED state.

In response to this:

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error

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.

openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels May 22, 2020

openshift-ci-robot requested review from abhi-kn and invincibleJai May 22, 2020 12:52

openshift-ci-robot added the component/dev-console Related to dev-console label May 22, 2020

openshift-ci-robot assigned andrewballantyne May 22, 2020

openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label May 22, 2020

andrewballantyne suggested changes May 22, 2020

View reviewed changes

karthikjeeyar force-pushed the pipeline-logs branch from 3f892c2 to 0d4b8f6 Compare May 23, 2020 06:10

openshift-ci-robot added the component/core Related to console core functionality label May 23, 2020

karthikjeeyar force-pushed the pipeline-logs branch from 0d4b8f6 to 9631b59 Compare May 23, 2020 06:11

andrewballantyne suggested changes May 25, 2020

View reviewed changes

karthikjeeyar force-pushed the pipeline-logs branch 2 times, most recently from 960c062 to 8527339 Compare May 26, 2020 11:42

andrewballantyne approved these changes May 26, 2020

View reviewed changes

openshift-ci-robot assigned christianvogt May 26, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2020

christianvogt reviewed May 26, 2020

View reviewed changes

fix empty {} Content in coFetchText causes PipelineLogs to show error

9d0e724

karthikjeeyar force-pushed the pipeline-logs branch from 8527339 to 9d0e724 Compare May 27, 2020 09:57

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label May 27, 2020

andrewballantyne approved these changes May 27, 2020

View reviewed changes

sahil143 reviewed May 27, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 27, 2020

openshift-merge-robot merged commit fc102b1 into openshift:master May 28, 2020

spadgett added this to the v4.5 milestone May 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error #5539

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error #5539

karthikjeeyar commented May 22, 2020

openshift-ci-robot commented May 22, 2020

karthikjeeyar commented May 22, 2020

andrewballantyne May 22, 2020

karthikjeeyar May 23, 2020

andrewballantyne left a comment

andrewballantyne May 25, 2020

karthikjeeyar May 26, 2020

andrewballantyne May 25, 2020

karthikjeeyar May 26, 2020

andrewballantyne May 26, 2020

andrewballantyne left a comment

christianvogt May 26, 2020

karthikjeeyar May 27, 2020

andrewballantyne left a comment

sahil143 left a comment

andrewballantyne commented May 27, 2020

sahil143 commented May 27, 2020

andrewballantyne commented May 27, 2020

christianvogt commented May 27, 2020

openshift-ci-robot commented May 27, 2020

openshift-ci-robot commented May 28, 2020

	return coFetchCommon(url, 'GET', options, timeout).then((response) => {
	return coFetchCommon(url, 'GET', options, timeout).then((response): string => {

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error #5539

Bug 1839080: fix empty Content in coFetchText causes PipelineLogs to show error #5539

Conversation

karthikjeeyar commented May 22, 2020

openshift-ci-robot commented May 22, 2020

karthikjeeyar commented May 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a 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

andrewballantyne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

sahil143 left a comment

Choose a reason for hiding this comment

andrewballantyne commented May 27, 2020

sahil143 commented May 27, 2020

andrewballantyne commented May 27, 2020

christianvogt commented May 27, 2020

openshift-ci-robot commented May 27, 2020

openshift-ci-robot commented May 28, 2020