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

JENKINS-49929 Regression: Log fails to auto update in karaoke mode #1686

Merged
merged 4 commits into from Mar 7, 2018

Conversation

sophistifunk
Copy link
Collaborator

Description

See JENKINS-49929.

Submitter checklist

  • Link to JIRA ticket in description, if appropriate.
  • Change is code complete and matches issue description
  • Appropriate unit or acceptance tests or explanation to why this change has no tests
  • Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • Run the changes and verified the change matches the issue description
  • Reviewed the code
  • Verified that the appropriate tests have been written or valid explanation given

@sophistifunk
Copy link
Collaborator Author

I haven't the foggiest idea how we would test this mechanically, only a certainly that writing a reliable ATH case for it would take 2 weeks.

@vivek vivek requested a review from kshultzCB March 6, 2018 18:16
Copy link
Collaborator

@NicuPascu NicuPascu left a comment

Choose a reason for hiding this comment

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

LGTM 🐝

@kzantow
Copy link
Collaborator

kzantow commented Mar 7, 2018

Prettier is not fun for changes like this, hard to see what the actual issue was, needs to be run against the whole codebase in one fell swoop and then applied during commit time as well

prefix: `step-${step.id}-`,
}}
/>);
children = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

oh thank God that spread syntax is gone

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Word. I kills it where I finds it.

} else if (step.isInputStep) {
children = <InputStep step={step} key="step" classicInputUrl={classicInputUrl} />;
} else if (!logArray && step.hasLogs) {
children = <span key={'span'}>&nbsp;</span>;
}
const getLogForNode = () => {
if (this.pager.log === undefined || (this.pager.log && !this.pager.log.data)) {
const cfg = { url: step.logUrl };
const cfg = { url: step.logUrl, followAlong: this.props.augmenter.karaoke };
Copy link
Collaborator

Choose a reason for hiding this comment

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

... wheeeeeeee karaoke!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, the other changes I fixed for readability while trying to understand the issue. Figured no reason to throw them away and have to do it again next week.

Copy link
Collaborator

@kzantow kzantow left a comment

Choose a reason for hiding this comment

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

LGTM, I finally found the change

@sophistifunk sophistifunk merged commit dd0959d into master Mar 7, 2018
@sophistifunk sophistifunk deleted the bug/JENKINS-49929-log-follow branch March 7, 2018 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants