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

Un-restrict the addCriticalField in XStream2 #3066

Merged
merged 2 commits into from Oct 13, 2017

Conversation

5 participants
@Wadeck
Contributor

Wadeck commented Oct 9, 2017

Like mentioned in the previous comment in code, the method should be used in the other project, like authorize-project that require a field to be set as critical.

As discussed with Jesse and Oleg on Oct. 5, I can un-restrict the call to this method by plugins.

You can see the workaround used in authorize-project at the moment : PR

See JENKINS-47342.

Proposed changelog entries

  • Un-restrict the use of the addCriticalField in XStream2 to be used in plugins

Desired reviewers

@reviewbybees

Like mentioned in the previous comment in code, the method should be …
…used in the other project, like authorize-project that require a field to be set as critical.

As discussed with Jesse and Oleg on Oct. 5, I can un-restrict the call to this method by plugins.
@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 9, 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.

@oleg-nenashev

A minor change in documentation is required.

@@ -131,7 +131,6 @@ protected Converter createDefaultConverter() {
* @param clazz a class which we expect to hold a non-{@code transient} field
* @param field a field name in that class
*/
@Restricted(NoExternalUse.class) // TODO could be opened up later

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Oct 9, 2017

Member

Javadoc needs to be updated in order to indicate a version which starts offering this API publicly. @since TODO is a recommended way, the core maintainers will update the version during the merge after that.

This comment has been minimized.

@Wadeck

Wadeck Oct 9, 2017

Contributor

@since TODO added in the javadoc

@oleg-nenashev

🐝 , I do not see any specific need to keep this field restricted (especially taking the comment into account).

Currently there is no good workaround for Jenkins plugins, only reflection or XStream override (the latter one is even worse)

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 9, 2017

Or a parent POM that ignores @Restricted 😭

I wonder whether our insane threaded use of XStream is a possible problem here. (I.e. do we need to document specifically how plugins should use this field so it's not randomly applied?)

@oleg-nenashev oleg-nenashev requested a review from jglick Oct 9, 2017

@jglick

This comment has been minimized.

Member

jglick commented Oct 9, 2017

do we need to document specifically how plugins should use this field so it's not randomly applied?

JENKINS-19561 is going to be a hassle either way; I do not think this would make it any worse.

@jglick

jglick approved these changes Oct 9, 2017

@daniel-beck

This comment has been minimized.

Member

daniel-beck commented Oct 9, 2017

I do not think this would make it any worse

I meant more like a guideline to use this only from e.g. PluginImpl, or how early in the life cycle the @Initializer must be, or that the plugin should declare itself to be not dynamically loadable, etc. – whatever makes this less errorprone, if necessary.

@Wadeck Wadeck self-assigned this Oct 12, 2017

@oleg-nenashev oleg-nenashev merged commit 7a0d73e into jenkinsci:master Oct 13, 2017

1 check failed

continuous-integration/jenkins/pr-head This commit has test failures
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment