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-33663] - Upgrade wizard #2281

Merged
merged 1 commit into from May 20, 2016

Conversation

10 participants
@kzantow
Contributor

kzantow commented Apr 21, 2016

Adapt the setup wizard GUI to provide a similar experience upgrading Jenkins.

This modifies / enhances the original banner to drop into an upgrade wizard with information about what's new in Jenkins 2 and providing a way to get the base set of plugins updated.

This addresses: https://issues.jenkins-ci.org/browse/JENKINS-33663

! NOTE: many of these files have existing whitespace issues, I plan on cleaning them all up after having the changes reviewed here

Screenshots:

image

image

image

image

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz Apr 22, 2016

Contributor

Manually tried. The upgrade window appears but it's blank. Browser console errors:

http://localhost:8080/adjuncts/d77aebba/jenkins/install/css/icons/icomoon.css Failed to load resource: the server responded with a status of 404 (Not Found)
http://localhost:8080/setupWizard/loadTemplate?templateName=upgradeTo20Panel&_=1461315124799 Failed to load resource: the server responded with a status of 404 (Not Found)
http://localhost:8080/adjuncts/d77aebba/jenkins/install/css/icons/icomoon.css Failed to load resource: the server responded with a status of 404 (Not Found)

I guess you already knew this (and that's why it's a WiP?).

Contributor

amuniz commented Apr 22, 2016

Manually tried. The upgrade window appears but it's blank. Browser console errors:

http://localhost:8080/adjuncts/d77aebba/jenkins/install/css/icons/icomoon.css Failed to load resource: the server responded with a status of 404 (Not Found)
http://localhost:8080/setupWizard/loadTemplate?templateName=upgradeTo20Panel&_=1461315124799 Failed to load resource: the server responded with a status of 404 (Not Found)
http://localhost:8080/adjuncts/d77aebba/jenkins/install/css/icons/icomoon.css Failed to load resource: the server responded with a status of 404 (Not Found)

I guess you already knew this (and that's why it's a WiP?).

/**
* Need InstallState != NEW for tests by default
*/
UNKNOWN(true, null),
@Extension

This comment has been minimized.

@amuniz

amuniz Apr 22, 2016

Contributor

@Extension in a class field? What's the effect of this?

@amuniz

amuniz Apr 22, 2016

Contributor

@Extension in a class field? What's the effect of this?

This comment has been minimized.

@amuniz

amuniz Apr 22, 2016

Contributor

I guess you want all this objects into the extensions container?

@amuniz

amuniz Apr 22, 2016

Contributor

I guess you want all this objects into the extensions container?

This comment has been minimized.

@kzantow

kzantow Apr 25, 2016

Contributor

Yes, exactly. I want the instances. It seems to work (and is documented to work this way) @amuniz

@kzantow

kzantow Apr 25, 2016

Contributor

Yes, exactly. I want the instances. It seems to work (and is documented to work this way) @amuniz

@@ -4587,6 +4580,26 @@ private static void computeVersion(ServletContext context) {
}
String ver = props.getProperty("version");
if(ver==null) ver = UNCOMPUTED_VERSION;
if(Main.isDevelopmentMode && "${build.version}".equals(ver)) {

This comment has been minimized.

@amuniz

amuniz Apr 22, 2016

Contributor

Why the second condition? Is isDevelopmentMode not enough?

@amuniz

amuniz Apr 22, 2016

Contributor

Why the second condition? Is isDevelopmentMode not enough?

This comment has been minimized.

@kzantow

kzantow May 13, 2016

Contributor

usually this is set, some cases (ahem debugging using eclipse) it is not

@kzantow

kzantow May 13, 2016

Contributor

usually this is set, some cases (ahem debugging using eclipse) it is not

Show outdated Hide outdated war/src/main/js/pluginSetupWizardGui.js
// indicates
var transitionPanel = function(state) {
if(/CREATE_ADMIN_USER/.test(state)) {

This comment has been minimized.

@amuniz

amuniz Apr 22, 2016

Contributor

Is === not valid here?

@amuniz

amuniz Apr 22, 2016

Contributor

Is === not valid here?

@kzantow kzantow changed the title from [JENKINS-33663] - Upgrade wizard WiP to [JENKINS-33663] - Upgrade wizard Apr 25, 2016

Show outdated Hide outdated core/src/main/java/jenkins/install/InstallState.java
/**
* Jenkins install state.
*
* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
*/
@Restricted(NoExternalUse.class)

This comment has been minimized.

@kzantow

kzantow Apr 25, 2016

Contributor

This may be referenced by extensions to add more install states

@kzantow

kzantow Apr 25, 2016

Contributor

This may be referenced by extensions to add more install states

Show outdated Hide outdated core/src/main/java/jenkins/install/InstallUtil.java
@@ -74,7 +74,7 @@
*/
public static InstallState getInstallState() {
// Support a simple state override. Useful for testing.
String stateOverride = System.getenv("jenkins.install.state");
String stateOverride = System.getProperty("jenkins.install.state", System.getenv("jenkins.install.state"));

This comment has been minimized.

@kzantow

kzantow Apr 25, 2016

Contributor

Easier to do -Djenkins.install.state=upgrade if doing development

@kzantow

kzantow Apr 25, 2016

Contributor

Easier to do -Djenkins.install.state=upgrade if doing development

This comment has been minimized.

@svanoort

svanoort Apr 26, 2016

Member

I like this. A lot.

@svanoort

svanoort Apr 26, 2016

Member

I like this. A lot.

This comment has been minimized.

@jtnord

jtnord Apr 28, 2016

Member

Windows development! because this will not work on Linux with Bash as you can not have environment variables with dots in them :-)

@jtnord

jtnord Apr 28, 2016

Member

Windows development! because this will not work on Linux with Bash as you can not have environment variables with dots in them :-)

iapf.touch(System.currentTimeMillis());
iapf.chmod(0640);
iapf.write(randomUUID + System.lineSeparator(), "UTF-8");
if(newInstall) {

This comment has been minimized.

@kzantow

kzantow Apr 25, 2016

Contributor

mostly this is just formatting here

@kzantow

kzantow Apr 25, 2016

Contributor

mostly this is just formatting here

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz Apr 25, 2016

Contributor

Re-tested after last modifications. All my manual tests passed.

Just one question; I saw only "Install Recommended Plugins" option available, is that right? Why I could not select what to install?

And one detail (but not sure if related to this PR); there is an empty space over the breadcrumb (is that intentional):

screenshot1

Contributor

amuniz commented Apr 25, 2016

Re-tested after last modifications. All my manual tests passed.

Just one question; I saw only "Install Recommended Plugins" option available, is that right? Why I could not select what to install?

And one detail (but not sure if related to this PR); there is an empty space over the breadcrumb (is that intentional):

screenshot1

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow Apr 26, 2016

Contributor

@amuniz the only options are to install the recommended plugins or decline. presumably they already have plugins installed and the plugin list isn't yet built to handle already installed plugins in a meaningful manner. the idea here is to get the recommended plugins installed as a baseline across all the Jenkins installations, or the existing ones will be missing out on some of the highlights of Jenkins 2. Regarding the space on the page, it's fixed, it was definitely an issue caused by this PR.

Contributor

kzantow commented Apr 26, 2016

@amuniz the only options are to install the recommended plugins or decline. presumably they already have plugins installed and the plugin list isn't yet built to handle already installed plugins in a meaningful manner. the idea here is to get the recommended plugins installed as a baseline across all the Jenkins installations, or the existing ones will be missing out on some of the highlights of Jenkins 2. Regarding the space on the page, it's fixed, it was definitely an issue caused by this PR.

@amuniz

This comment has been minimized.

Show comment
Hide comment
@amuniz

amuniz Apr 26, 2016

Contributor

Ok, thanks.
This is 👍 for me

Contributor

amuniz commented Apr 26, 2016

Ok, thanks.
This is 👍 for me

Show outdated Hide outdated core/src/main/java/jenkins/install/SetupWizard.java
jenkins.setInstallState(InstallState.CONFIGURE_SECURITY.getNextState());
return HttpResponses.okJSON();
}
return HttpResponses.errorJSON("Unable to configure security");

This comment has been minimized.

@svanoort

svanoort Apr 26, 2016

Member

Possibly a naive question here, but is there anything we need to revert in the event of a failure to configure the security?

@svanoort

svanoort Apr 26, 2016

Member

Possibly a naive question here, but is there anything we need to revert in the event of a failure to configure the security?

This comment has been minimized.

@kzantow

kzantow Apr 26, 2016

Contributor

@svanoort not really sure there's anything we can do in this case. this code wasn't changed, however just indented

@kzantow

kzantow Apr 26, 2016

Contributor

@svanoort not really sure there's anything we can do in this case. this code wasn't changed, however just indented

This comment has been minimized.

@svanoort

svanoort Apr 26, 2016

Member

@kzantow Yeah, I'm not exactly sure where this was originally touched, I'm just seeing it for the first time now and it's making my Spidey Sense tingle. Was this code path present before we added the SetupWizard somehow?

@svanoort

svanoort Apr 26, 2016

Member

@kzantow Yeah, I'm not exactly sure where this was originally touched, I'm just seeing it for the first time now and it's making my Spidey Sense tingle. Was this code path present before we added the SetupWizard somehow?

This comment has been minimized.

@kzantow

kzantow Apr 26, 2016

Contributor

@svanoort oh, wait a sec. yes, this was added as one of the derivations of the setup wizard, where security configuration is included as one of the steps... but then removed, now temporarily re-added as part 2 of the upgrade wizard is to deal with security, sorry I was talking about the other security saving spot re: indentation.

@kzantow

kzantow Apr 26, 2016

Contributor

@svanoort oh, wait a sec. yes, this was added as one of the derivations of the setup wizard, where security configuration is included as one of the steps... but then removed, now temporarily re-added as part 2 of the upgrade wizard is to deal with security, sorry I was talking about the other security saving spot re: indentation.

This comment has been minimized.

@kzantow

kzantow Apr 26, 2016

Contributor

... falling victim to the 'staring at this code too much' syndrome

@kzantow

kzantow Apr 26, 2016

Contributor

... falling victim to the 'staring at this code too much' syndrome

Show outdated Hide outdated war/src/main/js/util/jenkins.js
});
};
/**
* Load a handlebars template from the server
*/
exports.loadTemplate = function(templateName, handler, onError) {

This comment has been minimized.

@svanoort

svanoort Apr 26, 2016

Member

Maybe an 🐜 but I have to wonder if there isn't a better way to do this... perhaps by doing a little maven configuration to throw our JS/templates/etc resources in WEB-INF so they can be directly exposed from the WAR as simple URLs? Feels like we're reinventing the wheel by writing a custom function to serve static content using an URL param vs. using parts of the Servlet spec.

@svanoort

svanoort Apr 26, 2016

Member

Maybe an 🐜 but I have to wonder if there isn't a better way to do this... perhaps by doing a little maven configuration to throw our JS/templates/etc resources in WEB-INF so they can be directly exposed from the WAR as simple URLs? Feels like we're reinventing the wheel by writing a custom function to serve static content using an URL param vs. using parts of the Servlet spec.

This comment has been minimized.

@kzantow

kzantow Apr 26, 2016

Contributor

Yeah, agree.. this is actually not being used anyway, it was a byproduct of my first idea to make this happen... I'll remove it.

@kzantow

kzantow Apr 26, 2016

Contributor

Yeah, agree.. this is actually not being used anyway, it was a byproduct of my first idea to make this happen... I'll remove it.

This comment has been minimized.

@svanoort

svanoort Apr 26, 2016

Member

@kzantow Excellent - the removal commit for this looks good, btw

@svanoort

svanoort Apr 26, 2016

Member

@kzantow Excellent - the removal commit for this looks good, btw

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradePanel_banner=Welcome to Jenkins 2!
installWizard_upgradePanel_message=Jenkins 2 includes some great new features we think you'll love to use. Get your instance updated to the latest \
and greatest plugins available and take advantage of them!
installWizard_upgradePanel_recommendedActionTitle=Install Recommended Plugins

This comment has been minimized.

@rtyler

rtyler Apr 27, 2016

Member

s/Recommended/Suggested/

The terminology should be consistent (suggested) between the upgrade and new user flow.

@rtyler

rtyler Apr 27, 2016

Member

s/Recommended/Suggested/

The terminology should be consistent (suggested) between the upgrade and new user flow.

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
@@ -56,3 +58,17 @@ installWizard_pluginInstallFailure_message=Some plugins failed to install proper
with the failed plugins
installWizard_continue=Continue
installWizard_retry=Retry
installWizard_upgradePanel_title=Upgrade
installWizard_upgradePanel_banner=Welcome to Jenkins 2!
installWizard_upgradePanel_message=Jenkins 2 includes some great new features we think you'll love to use. Get your instance updated to the latest \

This comment has been minimized.

@rtyler

rtyler Apr 27, 2016

Member

"Get your instance updated" doesn't parse well IMHO. I suggest the text for this message to be:

Jenkins 2 includes some great new features that we think you'll love, install these additional plugins to take advantage of them!

@rtyler

rtyler Apr 27, 2016

Member

"Get your instance updated" doesn't parse well IMHO. I suggest the text for this message to be:

Jenkins 2 includes some great new features that we think you'll love, install these additional plugins to take advantage of them!

This comment has been minimized.

@batmat

batmat Apr 28, 2016

Member

@rtyler More a question from a non native speaker: didn't we say using the full form you will instead of you'll would be preferable in the UI texts?

Possibly I saw some fixes along those lines in the recent PRs @orrc did about improving many helps and so on.

@batmat

batmat Apr 28, 2016

Member

@rtyler More a question from a non native speaker: didn't we say using the full form you will instead of you'll would be preferable in the UI texts?

Possibly I saw some fixes along those lines in the recent PRs @orrc did about improving many helps and so on.

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradePanel_skipRecommendedPlugins=No thanks
installWizard_upgradeComplete_title=Upgrade
installWizard_pluginsInstalled_banner=Welcome to Jenkins 2!
installWizard_upgradeComplete_message=Congratulations! You've upgraded to Jenkins 2.</p><p>To learn more about the great new features, visit <a target="_blank" href="https://jenkins.io/2.0/">the Jenkins 2 website</a>!

This comment has been minimized.

@rtyler

rtyler Apr 27, 2016

Member

You've upgraded

Better just to write "You have upgraded", some people get weird about the contraction "you've"

@rtyler

rtyler Apr 27, 2016

Member

You've upgraded

Better just to write "You have upgraded", some people get weird about the contraction "you've"

This comment has been minimized.

@batmat

batmat Apr 28, 2016

Member

Hah, should have read that before maybe :)

@batmat

batmat Apr 28, 2016

Member

Hah, should have read that before maybe :)

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradeComplete_message=Congratulations! You've upgraded to Jenkins 2.</p><p>To learn more about the great new features, visit <a target="_blank" href="https://jenkins.io/2.0/">the Jenkins 2 website</a>!
installWizard_upgradeSkipped_title=Upgrade
installWizard_upgradeSkipped_banner=Features Not Installed
installWizard_upgradeSkipped_message=<div class="alert alert-warning fade in">You've decided to skip updating to the great features in Jenkins 2.</div></p><p><a target="_blank" href="https://jenkins.io/2.0/">Click here to learn what you're missing</a>!

This comment has been minimized.

@rtyler

rtyler Apr 27, 2016

Member

You've decided to skip updating to the great features in Jenkins 2.

I suggest just dropping this sentence, it comes across almost patronizing to me. Just the click prompt should be sufficient IMO

@rtyler

rtyler Apr 27, 2016

Member

You've decided to skip updating to the great features in Jenkins 2.

I suggest just dropping this sentence, it comes across almost patronizing to me. Just the click prompt should be sufficient IMO

Show outdated Hide outdated core/src/main/java/jenkins/install/SetupWizard.java
* Handle security configuration
*/
public HttpResponse doSaveSecurityConfig(StaplerRequest req, StaplerResponse rsp) throws IOException, ServletException, FormException {
jenkins.checkPermission(Jenkins.ADMINISTER); // this is redundant, maybe

This comment has been minimized.

@jtnord

jtnord Apr 28, 2016

Member

this is not redundant....

@jtnord

jtnord Apr 28, 2016

Member

this is not redundant....

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Apr 28, 2016

Member
  • Once I click the button, the overlay appears on every URL and I have no way to postpone the choice. I can only install, or decline, but not postpone in any way if I understand this approach correctly?
  • Showing the version number in the footer when upgrading makes no sense.
  • I don't know, the wording is still pretty… passive-aggressive, I guess? "Click here to learn what you're missing", why not start with this information to begin with, so I can make an informed choice?
Member

daniel-beck commented Apr 28, 2016

  • Once I click the button, the overlay appears on every URL and I have no way to postpone the choice. I can only install, or decline, but not postpone in any way if I understand this approach correctly?
  • Showing the version number in the footer when upgrading makes no sense.
  • I don't know, the wording is still pretty… passive-aggressive, I guess? "Click here to learn what you're missing", why not start with this information to begin with, so I can make an informed choice?
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Apr 28, 2016

Member
  • The "X" close button on the banner (which postpones for a day) is different from the "No thanks" option in the wizard itself.
  • Reusing the list of plugins from the initial setup makes no sense, as many of them are there to provide some basic functionality for Jenkins. If you're upgrading, it's safe to assume you already have that, so the 'exciting new features' should be limited to Pipeline and related plugins (and only if not already installed, surprise upgrades are not fun!), and not include Subversion plugin and LDAP plugin. There's also the problem of not being able to dynamically deploy because dependencies are already installed but in outdated versions.

That last item is a serious problem, so 👎 We haven't upgraded bundled Subversion Plugin to 2.0 for years because of the possible upgrade compatibility problem, and now we're doing that through the backdoor.

  • A proper solution to the upgrade problem will also need to take care of JENKINS-34494, for which we already received a number of reports.
Member

daniel-beck commented Apr 28, 2016

  • The "X" close button on the banner (which postpones for a day) is different from the "No thanks" option in the wizard itself.
  • Reusing the list of plugins from the initial setup makes no sense, as many of them are there to provide some basic functionality for Jenkins. If you're upgrading, it's safe to assume you already have that, so the 'exciting new features' should be limited to Pipeline and related plugins (and only if not already installed, surprise upgrades are not fun!), and not include Subversion plugin and LDAP plugin. There's also the problem of not being able to dynamically deploy because dependencies are already installed but in outdated versions.

That last item is a serious problem, so 👎 We haven't upgraded bundled Subversion Plugin to 2.0 for years because of the possible upgrade compatibility problem, and now we're doing that through the backdoor.

  • A proper solution to the upgrade problem will also need to take care of JENKINS-34494, for which we already received a number of reports.
@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Apr 28, 2016

Member

Some of the strings make no sense for an upgrade situation: "Start using Jenkins"?

Member

daniel-beck commented Apr 28, 2016

Some of the strings make no sense for an upgrade situation: "Start using Jenkins"?

/**
* Jenkins install state.
*
* @author <a href="mailto:tom.fennelly@gmail.com">tom.fennelly@gmail.com</a>
*/
@Restricted(NoExternalUse.class)
public enum InstallState {
public class InstallState implements ExtensionPoint {

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 6, 2016

Member

2.0 has been released.
Now it becomes a binary compatibility issue.

@daniel-beck Do we have a policy regarding it till 2.0 LTS?

@oleg-nenashev

oleg-nenashev May 6, 2016

Member

2.0 has been released.
Now it becomes a binary compatibility issue.

@daniel-beck Do we have a policy regarding it till 2.0 LTS?

This comment has been minimized.

@daniel-beck

daniel-beck May 6, 2016

Member

@oleg-nenashev It was @Restricted, so I don't understand what you're saying. This could as well be a different type, with larger changes to core to refer to the new type, and we could delete the old one.

@daniel-beck

daniel-beck May 6, 2016

Member

@oleg-nenashev It was @Restricted, so I don't understand what you're saying. This could as well be a different type, with larger changes to core to refer to the new type, and we could delete the old one.

This comment has been minimized.

@oleg-nenashev

oleg-nenashev May 11, 2016

Member

Yeah, no problem with the Restricted stuff

@oleg-nenashev

oleg-nenashev May 11, 2016

Member

Yeah, no problem with the Restricted stuff

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 6, 2016

Member

@kzantow This needs merge conflicts resolved.

Member

daniel-beck commented May 6, 2016

@kzantow This needs merge conflicts resolved.

@kzantow kzantow changed the title from [JENKINS-33663] - Upgrade wizard to [JENKINS-33663] - Upgrade wizard [WiP] May 12, 2016

@kzantow kzantow closed this May 13, 2016

@kzantow kzantow reopened this May 13, 2016

@kzantow kzantow changed the title from [JENKINS-33663] - Upgrade wizard [WiP] to [JENKINS-33663] - Upgrade wizard May 13, 2016

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 13, 2016

Member

Seems the bits from #2328 have not been integrated correctly.

From what I see in the manual tests, https://github.com/jenkinsci/jenkins/blob/master/war/src/main/js/util/jenkins.js#L212-213 will be removed if I merge this stuff as is. In such case the update center will be showing the offline pane if an update site skips a connectivity check.

Member

oleg-nenashev commented May 13, 2016

Seems the bits from #2328 have not been integrated correctly.

From what I see in the manual tests, https://github.com/jenkinsci/jenkins/blob/master/war/src/main/js/util/jenkins.js#L212-213 will be removed if I merge this stuff as is. In such case the update center will be showing the offline pane if an update site skips a connectivity check.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 13, 2016

Member

Removed bug, because it rather a jsbundles issue
"src//main/js/jsbundles/pluginWizard.js" is not being updated properly even during "mvn clean build". Smells like a bug in js-bundles (cc @tfennelly ). Had to delete the folder and rebuild it with Maven.

Member

oleg-nenashev commented May 13, 2016

Removed bug, because it rather a jsbundles issue
"src//main/js/jsbundles/pluginWizard.js" is not being updated properly even during "mvn clean build". Smells like a bug in js-bundles (cc @tfennelly ). Had to delete the folder and rebuild it with Maven.

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow May 13, 2016

Contributor

@oleg-nenashev #2328 changes are merged in this; they look the same as in the PR looking at my local copy

Contributor

kzantow commented May 13, 2016

@oleg-nenashev #2328 changes are merged in this; they look the same as in the PR looking at my local copy

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow May 17, 2016

Contributor

@mslusarczyk seems it was in error, I have some updates to this PR to address your feedback that will get pushed shortly

Contributor

kzantow commented May 17, 2016

@mslusarczyk seems it was in error, I have some updates to this PR to address your feedback that will get pushed shortly

DEFAULT: function() {
setPanel(welcomePanel);
// focus on default
$('.install-recommended').focus();

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

can welcomePanel name be overwritten? if so this will result in an exception.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

can welcomePanel name be overwritten? if so this will result in an exception.

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

No, welcomePanel is a local variable. But the DEFAULT state can be overwritten.

@kzantow

kzantow May 17, 2016

Contributor

No, welcomePanel is a local variable. But the DEFAULT state can be overwritten.

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

So what UpgradeWizard.java:55 is setting? Is it still used?

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

So what UpgradeWizard.java:55 is setting? Is it still used?

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

No, I'm getting rid of that, good catch. Simplifying things as much as I can.

@kzantow

kzantow May 17, 2016

Contributor

No, I'm getting rid of that, good catch. Simplifying things as much as I can.

Show outdated Hide outdated core/src/main/java/jenkins/install/InstallStateFilter.java
@@ -15,7 +15,7 @@
/**

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

Class javadoc is invalid there is no InstallUtil#getInstallState().

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

Class javadoc is invalid there is no InstallUtil#getInstallState().

Show outdated Hide outdated war/src/main/js/templates/pluginSelectionPanel.hbs
</div>
</div>
</div>
<div class="modal-footer">
<button type="button" class="btn btn-link install-home">
{{translations.installWizard_goBack}}
</button>
<button type="button" class="btn btn-primary install-selected">
<button id="install-plugins-button" type="button" class="btn btn-primary install-selected" {{#unless selectedPlugins}}disabled="disabled"{{/unless}}>

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

🐛 doesn't allow me to install 'none' plugins.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

🐛 doesn't allow me to install 'none' plugins.

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

You can click the X to close the wizard

@kzantow

kzantow May 17, 2016

Contributor

You can click the X to close the wizard

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

Right, but I'm skipping 'Create Admin User' this way also, correct?

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

Right, but I'm skipping 'Create Admin User' this way also, correct?

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

Good point, was mostly hoping to prevent accidentally clicking 'next' there

@kzantow

kzantow May 17, 2016

Contributor

Good point, was mostly hoping to prevent accidentally clicking 'next' there

final UpdateCenter updateCenter = jenkins.getUpdateCenter();
updateCenter.persistInstallStatus();
if (jenkins.getInstallState() == InstallState.NEW) {
if (!Jenkins.getInstance().getInstallState().isSetupComplete()) {
jenkins.setInstallState(InstallState.INITIAL_PLUGINS_INSTALLING);

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

hardcoded transition to INITIAL_PLUGINS_INSTALLING state.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

hardcoded transition to INITIAL_PLUGINS_INSTALLING state.

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

Right, this one should be. At this point, initial plugins are actually installing and providing feedback to the user.

@kzantow

kzantow May 17, 2016

Contributor

Right, this one should be. At this point, initial plugins are actually installing and providing feedback to the user.

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

OK, so why do we need separate NEW and INITIAL_PLUGINS_INSTALLING states if we are moving between them outside of InstallState flow.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

OK, so why do we need separate NEW and INITIAL_PLUGINS_INSTALLING states if we are moving between them outside of InstallState flow.

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

you might have different states between those states. this also allows refreshing the page to display the correct panel, regardless of the state you are currently on.

@kzantow

kzantow May 17, 2016

Contributor

you might have different states between those states. this also allows refreshing the page to display the correct panel, regardless of the state you are currently on.

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

This is fine after INITIAL_SECURITY_SETUP was added.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

This is fine after INITIAL_SECURITY_SETUP was added.

if (current == null || InstallState.UNKNOWN.equals(current)) {
return getDefaultInstallState();
}
final Map<InstallState, InstallState> states = new HashMap<InstallState, InstallState>();

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

this should be private static final in class

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

this should be private static final in class

This comment has been minimized.

@kzantow

kzantow May 17, 2016

Contributor

I'm not a fan of static state & a little memory can be reclaimed this way.

@kzantow

kzantow May 17, 2016

Contributor

I'm not a fan of static state & a little memory can be reclaimed this way.

Show outdated Hide outdated core/src/main/java/jenkins/install/SetupWizard.java
try {
PluginServletFilter.addFilter(FORCE_SETUP_WIZARD_FILTER);
} catch (ServletException e) {
throw new AssertionError(e);

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

why AssertionError? Also message detailing the exc would be nice.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

why AssertionError? Also message detailing the exc would be nice.

Show outdated Hide outdated war/pom.xml
@@ -676,6 +676,11 @@ THE SOFTWARE.
</file>
</activation>
<build>
<resources>
<resource>
<directory>${project.build.directory}/generated-resources/frontend</directory>

This comment has been minimized.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

is it still used? nothing is generated there.

@mslusarczyk

mslusarczyk May 17, 2016

Contributor

is it still used? nothing is generated there.

@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 17, 2016

Contributor

🐝

Contributor

mslusarczyk commented May 17, 2016

🐝

// If there are no platform updates, proceed to running
if (isUpToDate) {
if (Jenkins.getInstance().getSetupWizard().getPlatformPluginUpdates().isEmpty()) {
Jenkins.getInstance().setInstallState(InstallState.RUNNING);

This comment has been minimized.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

in InstallUtil there is states.put(InstallState.UPGRADE, InstallState.INITIAL_SETUP_COMPLETED);

why RUNNING is forced here?

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

in InstallUtil there is states.put(InstallState.UPGRADE, InstallState.INITIAL_SETUP_COMPLETED);

why RUNNING is forced here?

This comment has been minimized.

@kzantow

kzantow May 18, 2016

Contributor

because this is basically a restart with no valid upgrade, which would be "RESTART" or "RUNNING"

@kzantow

kzantow May 18, 2016

Contributor

because this is basically a restart with no valid upgrade, which would be "RESTART" or "RUNNING"

Show outdated Hide outdated core/src/main/java/jenkins/install/SetupWizard.java
@@ -404,6 +405,16 @@ void completeSetup() throws IOException {
}
/**
* Returns an installState by name
*/
public InstallState getInstallState(String name) {

This comment has been minimized.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

unused method.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

unused method.

Show outdated Hide outdated core/src/main/java/jenkins/install/SetupWizard.java
* Returns an installState by name
*/
public InstallState getInstallState(String name) {
if (name == null) {

This comment has been minimized.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

could be a part of InstallState.valueOf

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

could be a part of InstallState.valueOf

/**
* Returns an installState by name
*/
public InstallState getInstallState(String name) {

This comment has been minimized.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

unused method

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

unused method

This comment has been minimized.

@kzantow

kzantow May 18, 2016

Contributor

this is used from stapler

@kzantow

kzantow May 18, 2016

Contributor

this is used from stapler

Show outdated Hide outdated core/src/main/java/jenkins/install/InstallUtil.java
if (!current.equals(prior)) {
LOGGER.warning("Transitioning state from: " + prior + ", but current is: " + current);
}
InstallState next = getNextInstallState(prior);

This comment has been minimized.

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

does that overwrite 'current'? why do we need to define 'prior'?

@mslusarczyk

mslusarczyk May 18, 2016

Contributor

does that overwrite 'current'? why do we need to define 'prior'?

@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 18, 2016

Contributor

not a part of latest changes but PluginManager.java:673 should use equals.

Contributor

mslusarczyk commented May 18, 2016

not a part of latest changes but PluginManager.java:673 should use equals.

@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 18, 2016

Contributor

🐛 after the wizard is completed and Jenkins restarted I still see Unlock Jenkins screen. Seems that filter redirecting to it is not removed.

Contributor

mslusarczyk commented May 18, 2016

🐛 after the wizard is completed and Jenkins restarted I still see Unlock Jenkins screen. Seems that filter redirecting to it is not removed.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 18, 2016

Member

Findings in PR build 5255:

  • If after upgrading one installs only the Pipeline plugin (leaving GHOF uninstalled), the banner disappears.
  • If the Pipeline plugin was installed before upgrading, it's still offered in the wizard. If the user selects to install it, the progress screen hangs indefinitely.
  • The 'Back' button is somewhere completely differently from the 'Continue anyway' button when skipping installation, and basically impossible to notice on a large screen.
  • The 'X' button in the wizard behaves differently from the 'X' button in the banner, the latter should be replaced by something different.
  • When upgrading, the logging says it switches to InstallState (CREATE_ADMIN_USER) which makes no sense.
Member

daniel-beck commented May 18, 2016

Findings in PR build 5255:

  • If after upgrading one installs only the Pipeline plugin (leaving GHOF uninstalled), the banner disappears.
  • If the Pipeline plugin was installed before upgrading, it's still offered in the wizard. If the user selects to install it, the progress screen hangs indefinitely.
  • The 'Back' button is somewhere completely differently from the 'Continue anyway' button when skipping installation, and basically impossible to notice on a large screen.
  • The 'X' button in the wizard behaves differently from the 'X' button in the banner, the latter should be replaced by something different.
  • When upgrading, the logging says it switches to InstallState (CREATE_ADMIN_USER) which makes no sense.
@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 19, 2016

Contributor

still a 🐛. Important step to reproducing is to continue as a auto-generated admin on 'Create First Admin User' screen

Contributor

mslusarczyk commented May 19, 2016

still a 🐛. Important step to reproducing is to continue as a auto-generated admin on 'Create First Admin User' screen

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow May 19, 2016

Contributor

@daniel-beck your comments have been addressed here, please give the latest build a spin! :)
@mslusarczyk I've corrected the issue when 'continuing as admin' please let me know if there's anything else outstanding for your uses

Contributor

kzantow commented May 19, 2016

@daniel-beck your comments have been addressed here, please give the latest build a spin! :)
@mslusarczyk I've corrected the issue when 'continuing as admin' please let me know if there's anything else outstanding for your uses

body.no-decoration #side-panel,
body.no-decoration footer {
display: none;
}

This comment has been minimized.

@recena

recena May 19, 2016

Contributor

All of this will be removed when JENKINS-34670 is implemented.

@recena

recena May 19, 2016

Contributor

All of this will be removed when JENKINS-34670 is implemented.

This comment has been minimized.

@kzantow

kzantow May 19, 2016

Contributor

right, that's the idea @recena

@kzantow

kzantow May 19, 2016

Contributor

right, that's the idea @recena

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 19, 2016

Member

(Edit: Something went wrong with my default.json file, it appears to have been truncated)

FWIW plugins not found in the update site must never be suggested -- this will be relevant once plugins start depending on Jenkins 2.x (x ≠ 0), but users still install 2.0. Inapplicable suggestions (including those plugins marked as incompatible as either they or a dependency requires a newer core) must not be presented in upgrade or install wizards.

Otherwise, the 'minimize' button is a great improvement, and the 'no thanks' wording is much nicer.

Member

daniel-beck commented May 19, 2016

(Edit: Something went wrong with my default.json file, it appears to have been truncated)

FWIW plugins not found in the update site must never be suggested -- this will be relevant once plugins start depending on Jenkins 2.x (x ≠ 0), but users still install 2.0. Inapplicable suggestions (including those plugins marked as incompatible as either they or a dependency requires a newer core) must not be presented in upgrade or install wizards.

Otherwise, the 'minimize' button is a great improvement, and the 'no thanks' wording is much nicer.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 19, 2016

Member
  • Doesn't offer to install things that are already installed.
  • Doesn't offer to install things if upgrading from 2.x and not having them installed.
  • Doesn't show when everything that would be offered is already installed.

Very nice!

Making sure not to offer something not on the update site, or not compatible with the installed version, would be the only thing I noticed that would be left to do. (This can also happen with LTS releases, as everyone using 1.642.2 knows -- Pipeline currently requires 1.642.3 and shows as incompatible on .2)

Member

daniel-beck commented May 19, 2016

  • Doesn't offer to install things that are already installed.
  • Doesn't offer to install things if upgrading from 2.x and not having them installed.
  • Doesn't show when everything that would be offered is already installed.

Very nice!

Making sure not to offer something not on the update site, or not compatible with the installed version, would be the only thing I noticed that would be left to do. (This can also happen with LTS releases, as everyone using 1.642.2 knows -- Pipeline currently requires 1.642.3 and shows as incompatible on .2)

@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 19, 2016

Contributor

🐝

Contributor

mslusarczyk commented May 19, 2016

🐝

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradeSkipped_banner=Features Not Installed
installWizard_upgradeSkipped_message=<div class="alert alert-warning fade in">No new plugins will be installed</div> \
<p style="padding: 0 4px">You can also install new features from the Plugin Manager, if you change your mind.</p> \
<p style="padding: 0 4px">Visit the <a target="_blank" href="https://jenkins.io/2.0/">Jenkins 2 website</a> \

This comment has been minimized.

@rtyler

rtyler May 19, 2016

Member

Visit the Jenkins 2 website to learn how these new features can improve your Jenkins experience.

This sentence strikes me as awkward but I'm having difficulty explaining what it is exactly. If this were in documentation or a page on the site, I generally try to end the "call to action" with the link. I think that kind of sentence structure is more likely to get somebody to click, since they're not visually back-tracking to click the right link (perhaps I spend too much time thinking about these kinds of things 😄 )

I suggest:

Learn how these new features can improve your Jenkins experience on the Jenkins 2 homepage

@rtyler

rtyler May 19, 2016

Member

Visit the Jenkins 2 website to learn how these new features can improve your Jenkins experience.

This sentence strikes me as awkward but I'm having difficulty explaining what it is exactly. If this were in documentation or a page on the site, I generally try to end the "call to action" with the link. I think that kind of sentence structure is more likely to get somebody to click, since they're not visually back-tracking to click the right link (perhaps I spend too much time thinking about these kinds of things 😄 )

I suggest:

Learn how these new features can improve your Jenkins experience on the Jenkins 2 homepage

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradeComplete_message=Congratulations! You have upgraded to Jenkins {0}.</p><p>To learn more about the great new features, visit <a target="_blank" href="https://jenkins.io/2.0/">the Jenkins website</a>!
installWizard_upgradeSkipped_title=Upgrade
installWizard_upgradeSkipped_banner=Features Not Installed
installWizard_upgradeSkipped_message=<div class="alert alert-warning fade in">No new plugins will be installed</div> \

This comment has been minimized.

@rtyler

rtyler May 19, 2016

Member

No new plugins will be installed

I think that this could be confusing, I wouldn't want somebody to assume that the ability to install new plugins has been disabled.

I suggest:

Suggested plugins will not be installed.

@rtyler

rtyler May 19, 2016

Member

No new plugins will be installed

I think that this could be confusing, I wouldn't want somebody to assume that the ability to install new plugins has been disabled.

I suggest:

Suggested plugins will not be installed.

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradePanel_skipRecommendedPlugins=No thanks
installWizard_upgradeComplete_title=Upgrade
installWizard_pluginsInstalled_banner=Welcome to Jenkins {0}!
installWizard_upgradeComplete_message=Congratulations! You have upgraded to Jenkins {0}.</p><p>To learn more about the great new features, visit <a target="_blank" href="https://jenkins.io/2.0/">the Jenkins website</a>!

This comment has been minimized.

@rtyler

rtyler May 19, 2016

Member

Congratulations! You have upgraded to Jenkins {0}.To learn more about the great new features

IMO the last sentence should be more possessive

Congratulations! You have upgraded to Jenkins {0}. To learn more about _its_ great new features, visit foo

@rtyler

rtyler May 19, 2016

Member

Congratulations! You have upgraded to Jenkins {0}.To learn more about the great new features

IMO the last sentence should be more possessive

Congratulations! You have upgraded to Jenkins {0}. To learn more about _its_ great new features, visit foo

Show outdated Hide outdated core/src/main/resources/jenkins/install/pluginSetupWizard.properties
installWizard_upgradePanel_banner=Welcome to Jenkins {0}!
installWizard_upgradePanel_message=Jenkins {0} includes some great new features that we think you'll love, install these additional plugins to take advantage of them!
installWizard_upgradePanel_recommendedActionTitle=Install Suggested Plugins
installWizard_upgradePanel_recommendedActionDetails=Install the suggested plugins

This comment has been minimized.

@rtyler

rtyler May 19, 2016

Member

The redundancy here is somewhat goofy but I don't have a better suggestion at this time.

@rtyler

rtyler May 19, 2016

Member

The redundancy here is somewhat goofy but I don't have a better suggestion at this time.

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow May 19, 2016

Contributor

@daniel-beck I've added a test for a compatible version of the suggested plugins here ad454d3#diff-f65b8a70854ca1cc6c12397eee54d279R370

Contributor

kzantow commented May 19, 2016

@daniel-beck I've added a test for a compatible version of the suggested plugins here ad454d3#diff-f65b8a70854ca1cc6c12397eee54d279R370

@mslusarczyk

This comment has been minimized.

Show comment
Hide comment
@mslusarczyk

mslusarczyk May 19, 2016

Contributor

LGTM 🐝

Contributor

mslusarczyk commented May 19, 2016

LGTM 🐝

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck May 19, 2016

Member

Seems 👍

Member

daniel-beck commented May 19, 2016

Seems 👍

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev May 19, 2016

Member

After manual tests the behavior looks good to me. Hence 🐝 from me (I'm not the best code reviewer for this stuff BTW).

Member

oleg-nenashev commented May 19, 2016

After manual tests the behavior looks good to me. Hence 🐝 from me (I'm not the best code reviewer for this stuff BTW).

@recena

This comment has been minimized.

Show comment
Hide comment
@recena

recena May 19, 2016

Contributor

Me too, 🐝 some details can be polish later. Great work!

Contributor

recena commented May 19, 2016

Me too, 🐝 some details can be polish later. Great work!

@kzantow

This comment has been minimized.

Show comment
Hide comment
@kzantow

kzantow May 20, 2016

Contributor

sigh ... Finished: ABORTED

Contributor

kzantow commented May 20, 2016

sigh ... Finished: ABORTED

[FIX JENKINS-33663] - Upgrade wizard
Add Upgrade Wizard
Add 'replace' handlebars method in jenkins.js
Fix state transitions to follow a single basic pattern
Suggested plugins selected by default
Add an install state for INITIAL_SECURITY_SETUP
Removed duplicate setupWizard.js on page [FIX JENKINS-34676]
Exclude plugins which are already installed when determining platform
Change 'snooze' to not look like a 'close' button
Make sure unlock screen doesn't show up after new install
Add compatibility check when offering suggested plugins
More appropriate handling of security token filter

@kzantow kzantow merged commit 20cb8fd into jenkinsci:master May 20, 2016

1 check was pending

Jenkins Jenkins is validating pull request ...
Details

@kzantow kzantow deleted the kzantow:JENKINS-33663-upgrade-wizard branch May 20, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment