Skip to content

[JENKINS-70059] Improve Safe Restart interface and user information - #7355

Merged
timja merged 61 commits into
jenkinsci:masterfrom
meiswjn:feature/improve-safe-restart-messages
Jul 7, 2023
Merged

[JENKINS-70059] Improve Safe Restart interface and user information#7355
timja merged 61 commits into
jenkinsci:masterfrom
meiswjn:feature/improve-safe-restart-messages

Conversation

@meiswjn

@meiswjn meiswjn commented Nov 10, 2022

Copy link
Copy Markdown
Contributor

This PR aims to decrease user panic when a safe restart is performed.
See JENKINS-70059.

Testing done

Test it yourself

To test this locally, you will need a docker container. You can mount the war. Test it by running:
docker run -d --name jenkins -p 8080:8080 -v <path-to-your-war>/jenkins.war:/usr/share/jenkins/jenkins.war jenkins/jenkins

Afterwards, attach to the container to get the password:
docker exec -it jenkins /bin/bash

Testing done by me

Testing was only done manually inside a Docker container. I tested prepareShutdown and safeRestart via the UI of the Jenkins controller. I also tested the safe-restart CLI command, which still works without any parameters.

Screenshots

Safe Restart UI Before:

image

After:

  • New banner colour
  • Textbox for Reason (with default value)
  • Cancelable

image

Jenkins Unavailable (After Safe Restart) Before:

image

After:
image

Proposed changelog entries

  • Allow cancelling the quiet down mode of a safe restart with an optional custom message for safe restarts (with new default message). Use a less dangerous color for the safeRestart banner. Allow setting the full prepareShutdown message instead of only the reason. Show a hint on the "Jenkins Unavailable" page about safe restarts.

Proposed upgrade guidelines

N/A

Submitter checklist

  • The Jira issue, if it exists, is well-described.
  • The changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developers, depending on the change) and are in the imperative mood (see examples).
    • Fill in the Proposed upgrade guidelines section only if there are breaking changes or changes that may require extra steps from users during upgrade.
  • There is automated testing or an explanation as to why this change has no tests.
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadocs, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO"), if applicable.
  • New or substantially changed JavaScript is not defined inline and does not call eval to ease future introduction of Content Security Policy (CSP) directives (see documentation).
  • For dependency updates, there are links to external changelogs and, if possible, full differentials.
  • For new APIs and extension points, there is a link to at least one consumer.

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least two (2) approvals for the pull request and no outstanding requests for change.
  • Conversations in the pull request are over, or it is explicit that a reviewer is not blocking the change.
  • Changelog entries in the pull request title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood.
  • Proper changelog labels are set so that the changelog can be generated automatically.
  • If the change needs additional upgrade steps from users, the upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@meiswjn

meiswjn commented Nov 10, 2022

Copy link
Copy Markdown
Contributor Author

One of the changes will introduce this hint upon safe restarts:
image

@meiswjn meiswjn changed the title (WIP, but feedback welcome) Improve Safe Restart interface and user information Improve Safe Restart interface and user information Nov 11, 2022
@meiswjn
meiswjn marked this pull request as ready for review November 11, 2022 14:50
@DuMaM

DuMaM commented Nov 15, 2022

Copy link
Copy Markdown
Contributor

I relate to that description so much. When you have big instance (above 1K jobs), spam from users can be overwhelming. No matter what you say in communication channels, there will always somebody who will start screaming around in panic.
+1 for idea

Comment thread core/src/main/java/jenkins/model/Jenkins.java Outdated
@meiswjn
meiswjn requested a review from daniel-beck November 16, 2022 10:33
@meiswjn

meiswjn commented Nov 18, 2022

Copy link
Copy Markdown
Contributor Author

To test this locally, you will need a docker container. You can mount the war. Test it by running:
docker run -d --name jenkins -p 8080:8080 -v <path-to-your-war>/jenkins.war:/usr/share/jenkins/jenkins.war jenkins/jenkins

Afterwards, attach to the container to get the password:
docker exec -it jenkins /bin/bash

@daniel-beck daniel-beck left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like the idea of providing additional information during a restart a lot. And some UI that triggers safe restarts cannot easily be changed to have a custom message (e.g. the weird checkbox in the plugin manager/update center), so presenting it differently makes sense.

Some thoughts for your consideration:

  • Not all Jenkins installations can self-restart, so there's no way for admins to get the "gentle" message at all, even if they're manually doing the same thing.
  • The "gentle message" could just be a pre-determined custom message for the existing shutdown message.
  • Then custom shut-down message could be passed to HudsonIsRestarting and shown there.
  • It probably makes sense to make the existing "Jenkins is shutting down" message less panic-inducing, although we'll need to strike a balance to ensure it's still noticeable.

I think this would result in the same goals being met, while taking care of some configurations that might not be addressed by the current implementation.

Note that the above is preliminary feedback before I had an opportunity to run this, so may well be wrong. I would appreciate any corrections where I've misunderstood what this is doing.

Comment thread core/src/main/java/jenkins/model/Jenkins.java Outdated
Comment thread core/src/main/java/jenkins/model/Jenkins.java Outdated
Comment thread core/src/main/java/jenkins/model/Jenkins.java
Comment thread core/src/main/java/jenkins/model/Jenkins.java Outdated
Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com>
@meiswjn

meiswjn commented Nov 18, 2022

Copy link
Copy Markdown
Contributor Author

Hi @daniel-beck, thanks for your feedback. A couple of questions for clarification:

  • Not all Jenkins installations can self-restart, so there's no way for admins to get the "gentle" message at all, even if they're manually doing the same thing.

With 'manually', do you mean prepareShutdown? For this, the default message has been changed as well and it can be customized completely now. Or do you mean the orange design?

  • The "gentle message" could just be a pre-determined custom message for the existing shutdown message.

With pre-determined, do you mean a general configuration option? I thought about this but think it would be more intuitive to just pre-fill the reason field and if it is empty, provide a default.

  • Then custom shut-down message could be passed to HudsonIsRestarting and shown there.

HudsonIsRestarting is rendered when the UI is actually no longer available. Usually the reason was not presented there but merely an information that Jenkins will be back. I think it might be impractical to show this message here, since the message during pre-restart may not be accurate during the restart itself. However, it would be possible to pass a boolean to determine if it should show the green safe restart box I guess.

  • It probably makes sense to make the existing "Jenkins is shutting down" message less panic-inducing, although we'll need to strike a balance to ensure it's still noticeable.

I think this PR would fix this as well by allowing you to specify a custom message. There are a lot more ways, of course, but this would be a start.

@NotMyFault NotMyFault added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Nov 25, 2022
@meiswjn

meiswjn commented Dec 13, 2022

Copy link
Copy Markdown
Contributor Author

Hi @daniel-beck,

Since you are so busy, would it make more sense to give this review to another member of the core team? Same with #6539.

@meiswjn

meiswjn commented Jan 5, 2023

Copy link
Copy Markdown
Contributor Author

Hi @daniel-beck,
I appreciate your feedback so far. I hope you don't feel passed over when I request a review from some other contributors via Gitter since you seem to be very busy :)

@daniel-beck

Copy link
Copy Markdown
Member

@meiswjn No worries, I understand :) FWIW your other PR is still at the top of my list, but keeps getting pre-empted 😢

@daniel-beck

Copy link
Copy Markdown
Member

Perhaps @janfaracik could provide feedback on

Screenshot 2023-05-31 at 06 15 06

To me this just looks off. It reminds me of plugins like Ownership, that highlight their own content in a way that reduces consistency with the rest of the UI.


Nit: When I initiate a safe restart, and then update the cause by going to the "Prepare for shutdown" form, the banner changes color.

FWIW my feedback is (still) nonblocking. If there's enough approvals from others, this PR is free to be merged.

@meiswjn

meiswjn commented Jun 6, 2023

Copy link
Copy Markdown
Contributor Author

Hey @daniel-beck, thanks for your feedback! I am not really able to think of a more Jenkins-ish way of displaying it, I am afraid.

Since your feedback is non-blocking: Is it a bug on my side, or does the previous review from @NotMyFault still count towards the required reviewers?
image
If the review is still valid, isn't this PR ready to be merged because of the approvals of @timja and @NotMyFault?

@NotMyFault
NotMyFault requested a review from a team June 8, 2023 14:51
@NotMyFault NotMyFault changed the title Improve Safe Restart interface and user information [JENKINS-70059] Improve Safe Restart interface and user information Jun 8, 2023

@NotMyFault NotMyFault left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you so much for moving this forward!

If the review is still valid, isn't this PR ready to be merged because of the approvals of timja and NotMyFault?

I'd like to have @janfaracik take a look at the UI design and Daniel's comment above, if possible :)

@meiswjn

meiswjn commented Jun 19, 2023

Copy link
Copy Markdown
Contributor Author

@janfaracik Do you have a moment to comment on this PR? Would greatly appreciate it!

@janfaracik

Copy link
Copy Markdown
Member

LGTM.

Agree with @daniel-beck about the green banner, IMO it draws too much attention - but not a blocker.

On the 'Jenkins is restarting' screen the banner almost looks actionable/clickable, especially with the term 'Safe restart'? If the banner was replaced with some hint text, e.g. 'Jenkins is restarting. Builds can usually continue.' it'd be better. A user might not understand what a 'Safe restart' is.

The booting/restart/error screens could do with taking a look at in general so I wouldn't block this PR going forward.

@meiswjn

meiswjn commented Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

LGTM.

Agree with @daniel-beck about the green banner, IMO it draws too much attention - but not a blocker.

On the 'Jenkins is restarting' screen the banner almost looks actionable/clickable, especially with the term 'Safe restart'? If the banner was replaced with some hint text, e.g. 'Jenkins is restarting. Builds can usually continue.' it'd be better. A user might not understand what a 'Safe restart' is.

The booting/restart/error screens could do with taking a look at in general so I wouldn't block this PR going forward.

Thanks for having a look!
Since the plan is to reduce the panic of users ("Downtime??"), it is actually wanted to draw a bit of attention to it. While I agree that a user may not know what a safe restart is, the meaning is basically the sub-text: Builds can usually continue while Jenkins is restarting. The wording "safe restart", which is already an established Jenkins-term, is pretty nice for this as it communicates some safety and stability towards the user.

LGTM.

Since the review is non-blocking and two core maintainers approved the PR: Would it be possible to merge this?

@timja

timja commented Jun 26, 2023

Copy link
Copy Markdown
Member

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot Bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 26, 2023
@NotMyFault NotMyFault added major-rfe For changelog: Major enhancement. Will be highlighted on the top and removed rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted labels Jun 26, 2023
@NotMyFault

NotMyFault commented Jun 26, 2023

Copy link
Copy Markdown
Member

I would like to withdraw the ready-for-merge label temporarily, until the security team has reviewed this PR, given it's close to the finishing line.

@NotMyFault NotMyFault added the needs-security-review Awaiting review by a security team member label Jun 26, 2023
@NotMyFault
NotMyFault requested a review from a team June 26, 2023 16:28
@NotMyFault NotMyFault removed the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 26, 2023
Comment thread core/src/main/resources/hudson/cli/Messages.properties Outdated
@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jun 28, 2023
@timja

timja commented Jun 29, 2023

Copy link
Copy Markdown
Member

/label ready-for-merge


This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot Bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 29, 2023
@timja
timja merged commit df4972f into jenkinsci:master Jul 7, 2023
@welcome

welcome Bot commented Jul 7, 2023

Copy link
Copy Markdown

Congratulations on getting your very first Jenkins core pull request merged 🎉🥳

This is a fantastic achievement, and we're thrilled to have you as part of our community! Thank you for your valuable input, and we look forward to seeing more of your contributions in the future!

We would like to invite you to join the community chats and forums to meet other Jenkins contributors 😊
Don't forget to check out the participation page to learn more about how to contribute to Jenkins.


@meiswjn
meiswjn deleted the feature/improve-safe-restart-messages branch July 7, 2023 07:57
<strong>${%Safe Restart}</strong>
</p>
<p>
${%Builds on agents can usually continue.}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is it worth pointing out that this applies only to Pipeline builds, not to other project types?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good idea!
"Pipeline builds on agents can usually continue."

What would you think about that?

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

Labels

major-rfe For changelog: Major enhancement. Will be highlighted on the top ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants