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-44608] Don't check for monitor activation if it is disabled #2909

Merged
merged 2 commits into from Jun 3, 2017

Conversation

@andresrc
Copy link
Contributor

andresrc commented Jun 1, 2017

Description

See JENKINS-44608.

Details:

See ticket.

Changelog entries

Proposed changelog entries:

  • Administrative Monitors activation was checked even for disabled ones (issue 44608).

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: trivial change, tested manually that enable notifications appear both in the Manage Jenkins and the page decorator.
  • For new API and extension points: Link to the reference implementation in open-source (or example in Javadoc)

Desired reviewers

@reviewbybees

@reviewbybees

This comment has been minimized.

Copy link

reviewbybees commented Jun 1, 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.

@@ -60,10 +60,8 @@ THE SOFTWARE.
<l:main-panel>
<h1>${%Manage Jenkins}</h1>

<j:forEach var="am" items="${app.administrativeMonitors}">
<j:if test="${am.isActivated() and am.isEnabled()}">

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jun 1, 2017

Member

So all of this instead of

<j:if test="${am.isEnabled() and am.isActivated()}">

Why?

This comment has been minimized.

Copy link
@andresrc

andresrc Jun 1, 2017

Author Contributor

Yep. Initially I thought of simply doing that but given that I was going to create an issue, a PR and so on, I thought it was convenient to add the (still small) additional changes:

  • Use the same logic, from the same place in the manage Jenkins page and the decorator (the decorator will simply forward to Jenkins once the TODOs are solved).
  • Remove that logic from the Jelly.
/**
* Returns the enabled and activated administrative monitors.
*/
public List<AdministrativeMonitor> getActiveAdministrativeMonitors() {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jun 1, 2017

Member

New public unrestricted API without @since TODO.

This comment has been minimized.

Copy link
@andresrc

andresrc Jun 1, 2017

Author Contributor

Ups, will fix.

Copy link
Member

oleg-nenashev left a comment

LGTM 🐝

@andresrc

This comment has been minimized.

Copy link
Contributor Author

andresrc commented Jun 2, 2017

@daniel-beck daniel-beck merged commit 59ae1f6 into jenkinsci:master Jun 3, 2017
1 check passed
1 check passed
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
Projects
None yet
4 participants
You can’t perform that action at this time.