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.

Sign up for GitHub

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

Jump to bottom

Bug 1915898: Drop "undefined" and keep empty lines in Pipeline log output #7828

Merged
merged 1 commit into from Jan 22, 2021
Merged

Bug 1915898: Drop "undefined" and keep empty lines in Pipeline log output #7828

merged 1 commit into from Jan 22, 2021

Conversation

jerolimov
Copy link
Member

@jerolimov jerolimov commented Jan 13, 2021 •
edited

Fixes:
https://issues.redhat.com/browse/ODC-5224
https://bugzilla.redhat.com/show_bug.cgi?id=1915898

Analysis / Root cause:
There are some edge cases where the old case appends "undefined" to the log output. The old solution also drops empty lines.

Solution Description:
Drop the string operations which split strings and append them to different div elements. Don't add new div elements per line. (Pod logs also doesn't do this.) Just use one single "content" div where new content was appended to the innerText, incl. \n.

Added a second empty div as scroll references for autoscrolling.

Screen shots / Gifs for design review:

  1. Could not add a screenshot for a "undefined" which should not appear anymore. :)
  2. For the missing empty lines (see empty-lines.yaml) below the output changed from this:
    image

to this:
image

The tkn cli outputs a similar look and contains the new lines:
image

Unit test coverage report:
Not touched

Test setup:
To tests if the undefined doesn't appear anymore you need to run a pipeline with many output and delays multiple times. I could find this undefined in many cases when runing the undefined-test.yaml pipeline below.

To test the new lines you need a pipeline which creates one. One way is to disable the bash command output and run commands separated by an echo. See empty-lines.yaml pipeline below.

undefined-test.yaml 
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: empty-lines
spec:
  tasks:
    - name: undefined-test
      taskSpec:
        steps:
          - image: node
            name: step-name
            script: |
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
              sleep 1
              node --version
empty-lines.yaml 
apiVersion: tekton.dev/v1beta1
kind: Pipeline
metadata:
  name: empty-lines
spec:
  tasks:
    - name: task-name
      taskSpec:
        steps:
          - image: node
            name: step-name
            script: |
              set +x
              echo
              node --version
              sleep 1
              node --version
              sleep 1
              echo
              node --version
              sleep 1
              echo
              echo
              node --version
              sleep 1
              node --version
              sleep 1
              echo
              node --version
              sleep 1
              echo
              node --version
              sleep 1

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

The smooth scrolling doesn't work on Safari with and without this PR.

@jerolimov jerolimov changed the title Drop "undefined" from output and simplify log streaming Bug 1915898: Drop "undefined" from output and simplify log streaming Jan 13, 2021
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Jan 13, 2021
@openshift-ci-robot
Copy link
Contributor

@jerolimov: This pull request references Bugzilla bug 1915898, which is invalid:

  • expected the bug to target the "4.7.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1915898: Drop "undefined" from output and simplify log streaming

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 openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 13, 2021
@openshift-ci-robot
Copy link
Contributor

@jerolimov: This pull request references Bugzilla bug 1915898, which is invalid:

  • expected the bug to target the "4.7.0" release, but it targets "4.8.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1915898: Drop "undefined" from output and simplify log streaming

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.

@jerolimov
Copy link
Member Author

/kind bug
/assign @andrewballantyne

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 13, 2021
@jerolimov jerolimov changed the title Bug 1915898: Drop "undefined" from output and simplify log streaming Bug 1915898: Drop "undefined" and keep empty lines in Pipeline log output Jan 13, 2021
@jerolimov
Copy link
Member Author

/cc @karthikjeeyar

@jerolimov
Copy link
Member Author

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 19, 2021
@openshift-ci-robot
Copy link
Contributor

@jerolimov: This pull request references Bugzilla bug 1915898, 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.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

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 openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 19, 2021
@karthikjeeyar
Copy link
Contributor

Thanks @jerolimov for matching the log output with tkn cli log output and am not able to see the undefined issue anymore.
/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 20, 2021
andrewballantyne
andrewballantyne reviewed Jan 22, 2021
View reviewed changes
frontend/packages/pipelines-plugin/src/components/pipelineruns/logs/Logs.tsx
const appendMessage = React.useRef<(blockContent) => void>();
const prevFetchNewline = React.useRef(true);

appendMessage.current = React.useCallback(
(blockContent: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Logically, this method should be cleaned up more. Your two if-statements share the same logical check.

andrewballantyne
andrewballantyne approved these changes Jan 22, 2021
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Looks good, much cleaner logic. Only one nit, approving.

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, jerolimov, 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:

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 openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit bc44070 into openshift:master Jan 22, 2021
@openshift-ci-robot
Copy link
Contributor

@jerolimov: All pull requests linked via external trackers have merged:

Bugzilla bug 1915898 has been moved to the MODIFIED state.

In response to this:

Bug 1915898: Drop "undefined" and keep empty lines in Pipeline log output

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.

@spadgett spadgett added this to the v4.7 milestone Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewballantyne andrewballantyne andrewballantyne approved these changes

@christianvogt christianvogt Awaiting requested review from christianvogt

@vikram-raj vikram-raj Awaiting requested review from vikram-raj

@karthikjeeyar karthikjeeyar Awaiting requested review from karthikjeeyar

Assignees

@andrewballantyne andrewballantyne

@karthikjeeyar karthikjeeyar

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-low Referenced Bugzilla bug's severity is low 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. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.7
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@jerolimov @openshift-ci-robot @karthikjeeyar @andrewballantyne @spadgett @openshift-merge-robot