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

Display appropriate GUI that accurately displays offline by design #9883

Merged

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Oct 17, 2024

Reworked computer icon and associated descriptions to be controlled by the OfflineCause implementation. For implementations such as RetentionStrategy.Demand, this allows to set an offline cause that can't be interpreted by the end user as an error.

  • Allow the retention strategy to set an offline cause. For the on-demand retention strategy, this allows to provide a reason why the agent is offline, instead of leaving the agent offline with no message.
  • Introduced pause and computer-paused symbols, used by the on-demand retention strategy to indicate the computer is offline on purpose and is not in error state
  • Add getOfflineCause to IComputer and refactor related methods to pull up methods that make sense at the interface level.
  • Extract IOfflineCause interface from OfflineCause to make it possible to render offline causes in other runtimes (CloudBees CI HA)

Screenshots

Description Before After
On-demand agent offline before any connection attempt agent-ondemand-offline-before agent-ondemand-offline
Agent overview agent-set-overview-before agent-set-overview
Agent temporary offline agent-temporary-offline-user-before agent-temporary-offline-user
Agent tooltip when offline due to idle No tooltip agent-tooltip-idle
Agent tooltip when temporary offline No tooltip agent-tooltip-temporary-offline

See JENKINS-XXXXX.

Testing done

Proposed changelog entries

  • The agents online/offline status and icon can now be influenced by the offline cause, giving better information to users as well as clarifying some use cases when an agent was offline by design and not because of a configuration or technical error.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

Reworked computer icon and associated descriptions to be controlled by the `OfflineCause` implementation. For implementations such as `RetentionStrategy.Demand`, this allows to set an offline cause that can't be interpreted by the end user as an error.

Introduced `pause` and `computer-paused` symbols, used by the on-demand retention strategy to indicate the computer is offline on purpose and is not in error state
Add `getOfflineCause` to `IComputer` and refactor related methods to pull up methods that make sense at the interface level.
Extract `IOfflineCause` interface from `OfflineCause`.
@Vlatombe Vlatombe added the developer Changes which impact plugin developers label Oct 17, 2024
@Vlatombe Vlatombe requested a review from jglick October 17, 2024 08:49
@Vlatombe Vlatombe added rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted and removed developer Changes which impact plugin developers labels Oct 17, 2024
@mawinter69
Copy link
Contributor

Could you add the new icon to https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/hudson/model/ComputerSet/_legend.jelly
and probably update the not-accepting texts

@Vlatombe
Copy link
Member Author

I have never even seen that "legend" popup. TIL

@Vlatombe
Copy link
Member Author

probably update the not-accepting texts

Actually not needed. The existing on-demand retention strategy was putting the agent offline, whereas the current scheduled retention strategy just marks it as suspended (so it actually is connected but can't be used).

@Vlatombe Vlatombe requested review from jglick and a team October 18, 2024 09:23
@@ -212,5 +222,28 @@ public static class IdleOfflineCause extends SimpleOfflineCause {
public IdleOfflineCause() {
super(hudson.slaves.Messages._RetentionStrategy_Demand_OfflineIdle());
Copy link
Member

Choose a reason for hiding this comment

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

If this wasn't shown before and now is I'd suggest changing the text from computer to agent

Offline because computer was idle doesn't make sense as a computer isn't user facing terminology, (not mentioned in https://www.jenkins.io/doc/book/glossary/), it should be agent instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was already shown before, just not before the agent was online, then became offline again as instructed by the retention strategy.

@timja
Copy link
Member

timja commented Oct 18, 2024

can you add a changelog entry please

@Vlatombe Vlatombe requested a review from timja October 22, 2024 07:58
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

/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 Oct 22, 2024
@timja timja merged commit d9fbb65 into jenkinsci:master Oct 23, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants