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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-26940] Print message when installer isn't applicable #2598

Merged
merged 2 commits into from
Oct 25, 2016
Merged

[JENKINS-26940] Print message when installer isn't applicable #2598

merged 2 commits into from
Oct 25, 2016

Conversation

daniel-beck
Copy link
Member

Example:

[Gradle] - Launching build.
Installer "Extract *.zip/*.tar.gz" cannot be used to install "Gradle 2" on the node "Jenkins"
[Gradle] - [ERROR] Can't retrieve the Gradle executable.

It seems that users occasionally wonder why their installers "don't work". I'd also print the label expression to be extra helpful, but this may be misleading as isApplicable(…) can be overridden.

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Oct 20, 2016 •
edited
Loading

Weak 👎 for the current implementation, 👍 for working on it. I had many installers defined for Tool Installations (up to 16 IIRC for label combinations) + multiple tool definitions. Such per-installer logging would pollute my builds much by expected behavior.

My proposal:

  • Log info for each installer to system logs on FINE level
  • Write only one message to build logs if we're about returning null from the method ("None of installers can be applied")

@oleg-nenashev oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging labels Oct 20, 2016
oleg-nenashev
oleg-nenashev requested changes Oct 20, 2016
View reviewed changes
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.

needs change imho

@oleg-nenashev
Copy link
Member

oleg-nenashev commented Oct 20, 2016 •
edited
Loading

I'd vote for creating a new low-priority bug/improvement. May worth backporting

@daniel-beck
Copy link
Member Author

@ndeloof

Log info for each installer to system logs on FINE level

Uh, you mean log it in such a way that nobody will ever see it? What's the use of that? Since it's typically something trivial like labels, if you know how to look for the log you know enough to figure out the problem anyway.

Such per-installer logging would pollute my builds much by expected behavior.

How about this -- only print anything when no applicable installer was found, but then, print all of them? Then you'll only have log output in the failure case, i.e. when you need it.

@oleg-nenashev
Copy link
Member

@daniel-beck

How about this -- only print anything when no applicable installer was found, but then, print all of them? Then you'll only have log output in the failure case, i.e. when you need it.

Fine for me. FINE-level logging is not required in such case, of course

@daniel-beck daniel-beck added the needs-testcase Test automation is required for this pull request label Oct 20, 2016
@daniel-beck daniel-beck removed needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging needs-testcase Test automation is required for this pull request labels Oct 20, 2016
@daniel-beck
Copy link
Member Author

@oleg-nenashev Please re-review.

oleg-nenashev
oleg-nenashev reviewed Oct 20, 2016
View reviewed changes
core/src/main/java/hudson/tools/InstallerTranslator.java
}
}
for (String message : inapplicableInstallersMessages) {
log.getLogger().println(message);
Copy link
Member

Choose a reason for hiding this comment

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

Ideally it should be a single message for a correct logging. Especially once we have the External task logging support. Should be fine for now

Copy link
Member

Choose a reason for hiding this comment

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

We agreed to postpone it for now

@oleg-nenashev oleg-nenashev added ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback and removed needs-more-reviews Complex change, which would benefit from more eyes labels Oct 25, 2016
@oleg-nenashev oleg-nenashev merged commit ae29e6b into jenkinsci:master Oct 25, 2016
oleg-nenashev added a commit that referenced this pull request Oct 30, 2016
@oleg-nenashev
Changellog: Noting #2556, #2598, #2597, #2568, #2602, #2518, #2593
d5ffe44
olivergondza pushed a commit that referenced this pull request Nov 7, 2016
@daniel-beck @olivergondza
[JENKINS-26940] Print message when installer isn't applicable (#2598)
e85e3e2 
* [JENKINS-26940] Print message when installer isn't applicable

* [JENKINS-26940] Only print when none found, add test

(cherry picked from commit ae29e6b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@evieluvsrainbows evieluvsrainbows evieluvsrainbows approved these changes

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@daniel-beck @oleg-nenashev @evieluvsrainbows