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

[FIX JENKINS-41778] - setup wizard issues when failures #2825

Merged
merged 1 commit into from Apr 15, 2017

Conversation

5 participants
@kzantow
Contributor

kzantow commented Mar 30, 2017

Description

This corrects a few issues with the setup wizard: most notably, it gets stuck when plugins fail to install, also allows scrolling the mini-console and fixed the retry option

See JENKINS-41778.

Details: issues in setup wizard when failures occur. not really sure how to write tests for this, dependent on network issues or other failures that don't typically happen

Changelog entries

Proposed changelog entries:

  • Setup wizard gets into bad state when failures occur

Submitter checklist

  • JIRA issue is well described
  • Link to JIRA ticket in description, if appropriate
  • Appropriate autotests or explanation to why this change has no tests

Desired reviewers

@jenkinsci/code-reviewers
cc: @jtnord

setTimeout(attachScrollEvent, 50);
return;
}
var events = $._data($c[0], "events");

This comment has been minimized.

@kzantow

kzantow Mar 31, 2017

Contributor

this gets called a bunch of times, so only attach events if not already present

$c.scrollTop($c[0].scrollHeight);
}
// keep polling while install is running
if(complete < total || data.state === 'INITIAL_PLUGINS_INSTALLING') {
if(complete < total && data.state === 'INITIAL_PLUGINS_INSTALLING') {

This comment has been minimized.

@kzantow

kzantow Mar 31, 2017

Contributor

this was what was causing the 'hang'... everything done but since there were failures it didn't transition state

@kzantow kzantow requested a review from jtnord Apr 1, 2017

}
var events = $._data($c[0], "events");
if (!events || !events.scroll) {
$c.on('scroll', function() {

This comment has been minimized.

@scherler

scherler Apr 6, 2017

Contributor

https://developer.mozilla.org/en-US/docs/Web/Events/scroll

Since scroll events can fire at a high rate, the event handler shouldn't execute computationally expensive operations such as DOM modifications. Instead, it is recommended to throttle the event using requestAnimationFrame, setTimeout or customEvent

You should wrap everything that follows in

window.requestAnimationFrame(function() {
....
}

This comment has been minimized.

@kzantow

kzantow Apr 7, 2017

Contributor

This isn't performing DOM operations, just a simple check, it is not necessary.

This comment has been minimized.

@scherler

scherler Apr 10, 2017

Contributor

hmm, correct me if I am wrong but

var $c = $('.install-console-scroll');
...
$c.data('userScrolled', false);

counts as DOM manipulation but yeah.

The only thing that worries me that the the event can got fired a lot.

...but by no means I want to veto this PR. It was more an observation.

This comment has been minimized.

@kzantow

kzantow Apr 10, 2017

Contributor

No, it's a special jQuery data holder based on the element, it's not attr, which would add an attribute.

This comment has been minimized.

@kzantow

kzantow Apr 10, 2017

Contributor

... for clarification: the data method reads data-* attributes (see: https://api.jquery.com/data/#data-html5) it does not write them, so this would not be considered a DOM operation.

This comment has been minimized.

@scherler

scherler Apr 11, 2017

Contributor

Thanks for clarifying

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 7, 2017

Needs fix according to comment from @scherler

@kzantow

This comment has been minimized.

Contributor

kzantow commented Apr 7, 2017

@oleg-nenashev @scherler I don't think that change is necessary, as it's basically only doing a boolean check and setting a couple flags.

@rsandell

This comment has been minimized.

Member

rsandell commented Apr 10, 2017

🐝

@kzantow

This comment has been minimized.

Contributor

kzantow commented Apr 10, 2017

@jtnord @daniel-beck any qualms with merging this?

@kzantow kzantow removed the needs-fix label Apr 10, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 14, 2017

Formally @reviewbybees done

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 14, 2017

@jenkinsci/code-reviewers any concerns about merging? If no, I will merge it tomorrow to get into the weekly.

@reviewbybees

This comment has been minimized.

reviewbybees commented Apr 14, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Apr 15, 2017

Merging since there is no negative feedback after a week.

@oleg-nenashev oleg-nenashev merged commit ee82f61 into jenkinsci:master Apr 15, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment