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-23292] Add system property to disable jira mail address resolving #51

Merged
merged 5 commits into from Jun 12, 2015

Conversation

christ66
Copy link
Member

@christ66 christ66 commented Apr 3, 2015

Setting the system property -Dhudson.plugins.jira.JiraMailAddressResolver.DISABLE=true will disable resolving email addresses with Jira.

The reason for disabling the JiraMailAddressResolver is because it will access Jira to obtain the email address which could potentially return an invalid email (as in JENKINS-23292). Disabling the email resolver for Jira will allow the email addresses to be resolved through another method (possibly with LDAP).

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@olamy
Copy link
Member

olamy commented Apr 3, 2015

Sounds good.
But what about a configuration via the ui?

@daniel-beck
Copy link
Member

This is probably an unusual enough use case to not warrant a UI option.

*
* To disable set the System property "-Dhudson.plugins.jira.JiraMailAddressResolver.DISABLE=true"
*/
public static final boolean DISABLE = Boolean.getBoolean(JiraMailAddressResolver.class.getName() + ".DISABLE");
Copy link
Member

Choose a reason for hiding this comment

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

Why is this final?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since we are setting this argument through command line it shouldn't be modifiable. If I remove the final, then I can use a groovy script to set the value, and makes the argument useless.

Copy link
Member

Choose a reason for hiding this comment

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

then I can use a groovy script to set the value

Exactly. You'll be able to test whether this has the desired effect while troubleshooting without having to reboot the production instance.

If it does, you'd set the argument so Jenkins will retain that value in the future.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough. I'll remove final.

@walterk82
Copy link

What's the stopper on this pull request?

@artkoshelev
Copy link
Member

I don't think using class name in system property is a good idea. In case you rename this class, it will automatically change property name you read, so it will break functionality.

And please provide tests for your changes.

@daniel-beck
Copy link
Member

I don't think using class name in system property is a good idea.

This is an often-used pattern in Jenkins, see https://wiki.jenkins-ci.org/display/JENKINS/Features+controlled+by+system+properties

Backwards compatibility can easily be achieved by moving to a fixed string when renaming.

Note that plugins that depend on Jira Plugin today may also break in case you rename the class, as it is a public class.
https://wiki.jenkins-ci.org/display/JENKINS/Governance+Document#GovernanceDocument-Compatibilitymatters

…o test if this is the behavior they would like without restarting their Jenkins instance.
@christ66
Copy link
Member Author

Test has been added.


@BeforeClass
public static void setUp() throws Exception {
System.setProperty(JiraMailAddressResolver.class.getName() + ".DISABLE", "true");
Copy link
Member

Choose a reason for hiding this comment

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

If this used the full class name in a fixed string, it'd protect against class rename/move.

@walterk82
Copy link

Can we keep the progress moving on this? Should this be named JiraMailAddressResolver.class.getName() + ".disable" or JiraMailAddressResolver.class.getName() + ".disabled"

Thanks.

@artkoshelev
Copy link
Member

".disabled" sounds good to me

@walterk82
Copy link

I installed the snapshot and have confirmed that email resolving no longer occurs. I think it would be safe to merge this change.

rantoniuk pushed a commit that referenced this pull request Jun 12, 2015
[JENKINS-23292] Add system property -Dhudson.plugins.jira.JiraMailAddressResolver.disabled=true to disable jira mail address resolving
@rantoniuk rantoniuk merged commit 137cbc5 into jenkinsci:master Jun 12, 2015
@christ66 christ66 deleted the JENKINS-23292 branch August 27, 2015 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants