Swappable HTTP Client

[sghill]

This is a possible implementation of #272.

Here's a quick drawing of how this changes the plugin. I think it's 100% backwards-compatible, but allows for other plugins to change how notifications are sent without job configuration changing.

HttpNotifier

The first change is to separate the Apache HTTP Client implementation from what the plugin is doing. I left all the old methods intact so there could be one release with a deprecation, but I'm happy to remove them if you'd prefer. I called this interface HttpNotifier. The implementation included in the plugin (DefaultApacheHttpNotifier) is virtually unchanged, differing only to support the more generic method signatures.

HttpNotifierSelector

The second change introduces a HttpNotifierSelector. This is a layer that allows for doing live migrations to a running system. I have an implementation in another plugin that checks a list for jobs that are "enrolled" in the migration I'm doing. If a job is in the list, it gets my CustomHttpNotifier with different settings. If the job is not enrolled, it gets the default implementation. If there is some problem, I'm able to unenroll it without disrupting anyone and investigate what else needs to happen first while the build continues on the default notifier.

The implementation included in the plugin always returns DefaultApacheHttpNotifier for backwards compatibility.

Implementing A Custom HttpNotifierSelector

Here's how I have implemented a custom selector:

class CustomHttpNotifierSelector implements HttpNotifierSelector {
    private static final Logger log = LoggerFactory.getLogger(CustomHttpNotifierSelector.class);
    private final HttpNotifierSelector defaultSelector;
    private final HttpNotifier custom;
    private final UseNewClientRegistry registry;

    CustomHttpNotifierSelector(HttpNotifierSelector defaultSelector, HttpNotifier custom, UseNewClientRegistry registry) {
        this.defaultSelector = defaultSelector;
        this.custom = custom;
        this.registry = registry;
    }

    @NonNull
    @Override
    public HttpNotifier select(@NonNull SelectionContext selectionContext) {
        String fullName = selectionContext.getJobFullName();
        if (registry.isRegistered(fullName)) {
            log.info("{} is opted into custom client", fullName);
            return custom;
        }
        return defaultSelector.select(selectionContext);
    }
}

To use a custom HttpNotifierSelector, Jenkins has to know about it. This means it can either be in a Guice module tagged with the hudson.Extension annotation, or the class can be tagged directly with hudson.Extension. I think Guice makes testing much more friendly, so that's the approach I'm using:

@Extension
public class CustomStashNotifierModule extends AbstractModule {
    @Provides
    @Singleton
    @Named("custom")
    HttpNotifierSelector providesCustomHttpNotifierSelector(@StashNotifierDefault HttpNotifierSelector defaultSelector,
                                                            @Stash HttpNotifier customHttpNotifier,
                                                            UseNewClientRegistry registry) {
        return new CustomHttpNotifierSelector(defaultSelector, customHttpNotifier, registry);
    }
}

Note it's easy to inject the default HttpNotifierSelector for fallback scenarios.

Choosing A Custom HttpNotifierSelector Implementation

To choose an alternate implementation, the system property org.jenkinsci.plugins.stashNotifier.HttpNotifierSelector.className must be set to the fully qualified class name of the custom class. Jenkins will log which implementation its using when the first build using stashNotifier runs. Here's an example from my system:

INFO  o.j.p.s.StashNotifierModule - Using com.example.jenkins.stashNotifier.CustomHttpNotifierSelector

If a user happens to define a class name that cannot be found, it will log a warning and fall back to the default implementation:

WARN  o.j.p.s.StashNotifierModule - com.example.something.MissingSelector not found - using org.jenkinsci.plugins.stashNotifier.DefaultHttpNotifierSelector

I chose the system property approach because this is what Jenkins does for custom PluginManager implementations.

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[scaytrase]

Thanks @sghill. I'll take a deep look on this, but I can't promise this will be fast

[sghill]

Sounds good @scaytrase. Let me know if I can help.

[scaytrase]

👍 LGTM. Is it supposed to be drop-in replacement?

[sghill]

@scaytrase yes, it should function the same.

Users like me who want something else need to:

1. write another plugin that depends on this plugin
2. implement the new interface(s) in the other plugin
3. set the org.jenkinsci.plugins.stashNotifier.HttpNotifierSelector.className system property to tell Jenkins to use the new implementation

[scaytrase]

I have no way to test this, since I've no own installation of Jenkins today, so I have to trust my eyes here. This looks reasonable and good for me. I'll merge this and try to release this as fast as I can, maybe in a few days, maybe tomorrow, cant be sure

[scaytrase]

Thanks @sghill

[sghill]

Hi @scaytrase, thanks for the review and the merge. Do you think this week is looking likely for a release?

[scaytrase]

I've encountered some problems releasing this. Trying to sort thing out, I've lost my local release setup from last release. Sounds like some unreleased changes break release flow

[scaytrase]

Looks like I've messed up JDK installations. Can you check that new version is available?

[sghill]

I don't think 1.21 made it unfortunately. The last version I see in Jenkins Artifactory is 1.20.

[scaytrase]

That's bad. Will try again in the evening

[scaytrase]

1.22 appeared on your link. can you check it?

[sghill]

Confirmed 1.22 is released and working. I ran a test build with the default notifier and it posted to Bitbucket successfully. Thanks!

[scaytrase]

Thank you too for your contribution