Stop warning about not keeping undefined parameters #2687

Merged
merged 4 commits into from Jan 7, 2017

Conversation

3 participants
@sereinity
Contributor

sereinity commented Dec 30, 2016

On job chain, jenkins can "pass all current variables" to the next job.

The next job will (by default and this is good) consume only the variables that are defined as parameters.

Currently, when this appends it will write a warning. As we use a lot this kind of chaining, Jenkins produce a lot of logs (sometimes 3Gb a day) with almost only this warning.

The MR adds a parameter to jenkins hudson.model.ParametersAction.dontKeepUndefinedParameters (default false) which explicitly said that this behavior is wanted and stop produce warnings.

@oleg-nenashev

Looks reasonable 👍

@@ -75,6 +75,10 @@
public static final String SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +
".safeParameters";
+ @Restricted(NoExternalUse.class)
+ public static final String DONT_KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Dec 30, 2016

Member

Some Javadoc is would be useful here (though I cannot request it since properties above do not have it). E.g. brief description of the behavior and @since TODO

@oleg-nenashev

oleg-nenashev Dec 30, 2016

Member

Some Javadoc is would be useful here (though I cannot request it since properties above do not have it). E.g. brief description of the behavior and @since TODO

This comment has been minimized.

@sereinity

sereinity Dec 30, 2016

Contributor

done

@sereinity

sereinity Dec 30, 2016

Contributor

done

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Dec 31, 2016

Member

Can't we just use the existing property and make it tri-state? True, false, undefined? E.g. jenkins.install.runSetupWizard already works like this.

Member

daniel-beck commented Dec 31, 2016

Can't we just use the existing property and make it tri-state? True, false, undefined? E.g. jenkins.install.runSetupWizard already works like this.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jan 1, 2017

Member

Can't we just use the existing property and make it tri-state?

it may be possible, but IMHO no strong requirement.

Member

oleg-nenashev commented Jan 1, 2017

Can't we just use the existing property and make it tri-state?

it may be possible, but IMHO no strong requirement.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jan 1, 2017

Member

it may be possible, but IMHO no strong requirement.

If would at least prevent the awkwardness of defining both parameters and wondering what's going on.

Member

daniel-beck commented Jan 1, 2017

it may be possible, but IMHO no strong requirement.

If would at least prevent the awkwardness of defining both parameters and wondering what's going on.

@sereinity

This comment has been minimized.

Show comment
Hide comment
@sereinity

sereinity Jan 3, 2017

Contributor

@daniel-beck it's done.

Contributor

sereinity commented Jan 3, 2017

@daniel-beck it's done.

+ * If null, and they are not safe, it will log a warning in logs to the user
+ * to let him choose the behavior
+ *
+ * @since TODO

This comment has been minimized.

@daniel-beck

daniel-beck Jan 3, 2017

Member

Merge/cleanup note: Supported setting to "true" from 1.651.2 / 2.3, supported setting "false" from TODO.

@daniel-beck

daniel-beck Jan 3, 2017

Member

Merge/cleanup note: Supported setting to "true" from 1.651.2 / 2.3, supported setting "false" from TODO.

LOGGER.log(Level.WARNING, "Skipped parameter `{0}` as it is undefined on `{1}`. Set `-D{2}=true` to allow "
+ "undefined parameters to be injected as environment variables or `-D{3}=[comma-separated list]` to whitelist specific parameter names, "
- + "even though it represents a security breach",
+ + "even though it represents a security breach or `-D{2}=false to explicitly forbid it without warning.",

This comment has been minimized.

@daniel-beck

daniel-beck Jan 3, 2017

Member

Misses closing backtick.

Would make it a second sentence: Set -D{2}=false to no longer show this message.

@daniel-beck

daniel-beck Jan 3, 2017

Member

Misses closing backtick.

Would make it a second sentence: Set -D{2}=false to no longer show this message.

This comment has been minimized.

@sereinity

sereinity Jan 3, 2017

Contributor

fixed.

@sereinity

sereinity Jan 3, 2017

Contributor

fixed.

@daniel-beck

This comment has been minimized.

Show comment
Hide comment
@daniel-beck

daniel-beck Jan 7, 2017

Member

@oleg-nenashev Could you re-review and then merge if you approve?

Member

daniel-beck commented Jan 7, 2017

@oleg-nenashev Could you re-review and then merge if you approve?

@oleg-nenashev

I agree with the change since SystemProperties explicitly do not guarantee the backward compatibility

@oleg-nenashev oleg-nenashev merged commit c693f9e into jenkinsci:master Jan 7, 2017

2 checks passed

Jenkins This pull request looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

daniel-beck added a commit that referenced this pull request Jan 8, 2017

@sereinity sereinity deleted the sereinity:stop-warning-on-unkeeped-parameters branch Jan 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment