Skip to content

Add a check to avoid JENKINS-38664#22

Closed
hogarth-sv wants to merge 1 commit intojenkinsci:masterfrom
hogarth-sv:debug_status_notifier_400_error
Closed

Add a check to avoid JENKINS-38664#22
hogarth-sv wants to merge 1 commit intojenkinsci:masterfrom
hogarth-sv:debug_status_notifier_400_error

Conversation

@hogarth-sv
Copy link

This PR adds a check to ensure the Jenkins RootURL is configured and if it is not to skip the status notifier, rather than failing the build.

This mitigates JENKINS-38664 since it no longer causes the build to fail if the admin hasn't explicitly configured the Jenkins root URL.

The log results in this instead of the failure in the issue:

[Bitbucket] Notifying commit build result
Can not determine Jenkins root URL. Commit status notifications are disabled until a root URL is configured in Jenkins global configuration.

String url;
try {
url = DisplayURLProvider.get().getRunURL(build);
if (url.startsWith("http://unconfigured-jenkins-location/")) {
Copy link

Choose a reason for hiding this comment

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

It may be better to test if the domain name is a FQDN

URI testURI = new URI(url);
if (!testURI.getHost().contains(".")) {
    throw new IllegalStateException("Jenkins RootURL not a FQDN");
}

@hogarth-sv hogarth-sv force-pushed the debug_status_notifier_400_error branch from 8a67082 to 6365b2a Compare February 10, 2017 12:34
@hogarth-sv
Copy link
Author

Sorry I missed your review before.

I agree it's better to check fqdn than to rely on the output of the DisplayURLProvider plugin for an unconfigured jenkins instance.

I've rebased against master and adjusted to instead do as you suggest.

@hogarth-sv hogarth-sv force-pushed the debug_status_notifier_400_error branch from 6365b2a to f47879f Compare March 15, 2017 12:05
@hogarth-sv hogarth-sv force-pushed the debug_status_notifier_400_error branch from f47879f to 92d0b37 Compare March 15, 2017 12:12
@hogarth-sv
Copy link
Author

Rebased against current master to resolve conflicts from recent changes

@stephenc
Copy link
Member

Will consider after #53

@stephenc
Copy link
Member

Superceded by #97

@stephenc stephenc closed this Dec 18, 2017
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.

3 participants