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-30101] avoid temporarily offline cause is overwritten #6152

Open
wants to merge 42 commits into
base: master
Choose a base branch
from

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Jan 2, 2022

See JENKINS-30101.
See JENKINS-30175.
See JENKINS-50313.

When taking an agent temporarily offline the reason why it was done was
overwritten when the connection gets lost. Separate the temporarily offline
cause from those and display both in case they differ.

Testing

Besides the unit tests, tested manually that the offline causes are both shown.
Marking offline now shows the messaged Marked temporarily offline by while disconnecting stays with
DIsconnected by

Proposed changelog entries

  • Temporarily offline reason is not overwritten by other offline causes

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

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

  • There are at least 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 does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are correct
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (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).

When taking an agent temporarily offline the reason why it was done was
lost when the connection gets lost. Separate the temporarily offline
cause from those and display both.

Fixes: JENKINS-30101, JENKINS-30175 and JENKINS-50313
@daniel-beck daniel-beck self-requested a review January 2, 2022 16:29
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

Overall looks good and reasonable. At the same time we need to discuss the desired behavior of getOfflineCauseReason()

core/src/main/resources/hudson/model/Computer/index.jelly Outdated Show resolved Hide resolved
core/src/main/java/hudson/model/Computer.java Outdated Show resolved Hide resolved
@oleg-nenashev oleg-nenashev requested a review from a team January 7, 2022 11:40
@oleg-nenashev oleg-nenashev added the bug For changelog: Minor bug. Will be listed after features label Jan 7, 2022
@mawinter69
Copy link
Contributor Author

I don't think that the computer class should use the newly introduced method. It is not required. The existence of the temporarilyofflinecause in the Node class is so that after a restart Jenkins is able to restore the offline cause. But the actual handling of the causes is done in the Computer class and also the UI interacts with Computer and not Node.

@basil basil requested a review from a team April 15, 2022 17:31
@basil basil added the needs-more-reviews Complex change, which would benefit from more eyes label May 31, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 28, 2022
@github-actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 28, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 10, 2022
@github-actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Oct 11, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Dec 31, 2022
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 3, 2023
@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 4, 2023
@mawinter69
Copy link
Contributor Author

@jenkinsci/core friendly reminder

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 14, 2023
@github-actions
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jul 14, 2023
@mawinter69
Copy link
Contributor Author

@jenkinsci/core-pr-reviewers friendly reminder

Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Nov 24, 2023
@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Nov 25, 2023
@mawinter69
Copy link
Contributor Author

@basil @jenkinsci/core-pr-reviewers
It's now over 2 years that this PR is open. And the problem is still annoying. I would really like to get this merged.

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.

LGTM thanks, apologies for the delay

@timja timja requested review from a team and basil January 29, 2024 22:04
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 31, 2024
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Jan 31, 2024
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 18, 2024
Copy link

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features needs-more-reviews Complex change, which would benefit from more eyes
Projects
None yet
4 participants