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-48593] - Add getSystemProperty(key) to fix hudson.consoleTailKB system property #3200

Merged
merged 4 commits into from Dec 22, 2017

Conversation

3 participants
@jan-z
Copy link
Contributor

commented Dec 17, 2017

The system property "hudson.consoleTailKB" is supposed to control how many KB of console log is shown in the default console view.

It is used in two places:

https://github.com/jenkinsci/jenkins/blob/master/core/src/main/resources/hudson/model/Run/console.jelly#L38
https://github.com/jenkinsci/workflow-support-plugin/blob/master/src/main/resources/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl/index.jelly#L39

Both jelly scripts expect a method getSystemProperty(key) on the Functions class. But that method does not exist. Therefore the default of 150 KB is always used and the system property is ignored.

This PR adds this missing method to fix the system property "hudson.consoleTailKB".

@@ -469,6 +469,10 @@ public static Map getSystemProperties() {
return new TreeMap<Object,Object>(System.getProperties());
}

public static String getSystemProperty(String key) {

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Dec 17, 2017

Member

Lacks Javadoc, especially @since. Should probably be @Restricted.

@daniel-beck
Copy link
Member

left a comment

Would prefer an implementation that integrates with SystemProperties, otherwise that system erodes.

@jan-z

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

I didn't know that SystemProperties exists. Changed as requested.

@daniel-beck
Copy link
Member

left a comment

Looks reasonable, if a bit optimistic with the @since. Prepare to make it @since TODO tomorrow 😉

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

Since this fixes a bug, please search for existing reports in Jira, and if you don't find one, please file a new issue against core.

@jan-z

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2017

@oleg-nenashev oleg-nenashev changed the title Add getSystemProperty(key) to fix hudson.consoleTailKB system property [JENKINS-48593] - Add getSystemProperty(key) to fix hudson.consoleTailKB system property Dec 18, 2017

@oleg-nenashev
Copy link
Member

left a comment

👍 , just minor comments about annotations

*
* @since TODO
*/
@Restricted(NoExternalUse.class)

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Dec 18, 2017

Member

I would say DoNotUse.class is enough. We do not want this method to be used internally as well.

This comment has been minimized.

Copy link
@jan-z

jan-z Dec 19, 2017

Author Contributor

Changed as requested

*
* Delegates to {@link SystemProperties#getString(java.lang.String)}.
*
* @since TODO

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Dec 18, 2017

Member

Generally it's not required for restricted methods.
Especially in this case, I suspect the method has been removed at some point

@oleg-nenashev oleg-nenashev merged commit 006c512 into jenkinsci:master Dec 22, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Dec 22, 2017

Thanks @jan-z!

olivergondza added a commit that referenced this pull request Jan 3, 2018

[JENKINS-48593] - Add getSystemProperty(key) to fix hudson.consoleTai…
…lKB system property (#3200)

* Add getSystemProperty(key) to fix hudson.consoleTailKB system property

* Add Javadoc, @restricted and delegate to SystemProperties

These changes were requested by @daniel-beck.

* PR was not merged in time for 2.96

* Changes as requested by @oleg-nenashev

(cherry picked from commit 006c512)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.