Reduce usage of deprecated BitbucketRepositoryType.

[falkena]

This PR is a follow up from #518 and #520

Signed-off-by: Alexander Falkenstern [email protected]

Your checklist for this pull request

• Make sure you are requesting to pull a topic/feature/bugfix branch (right side) and not your master branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or in Jenkins JIRA
• Link to relevant pull requests, esp. upstream and downstream changes
• Did you provide a test-case? That demonstrates feature works or fixes the issue.

[falkena]

@KalleOlaviNiemitalo would you take a look please? I think, i removed this time usage properly. See #518 (comment) and #518 (comment)

[falkena]

Seems to be ok: https://stackoverflow.com/questions/16261383/delete-field-from-old-java-class-implementing-serializable/16261562

[KalleOlaviNiemitalo]

Fine then. If a user installs a new version of the plugin and the results are serialised without repositoryType, and the user then downgrades the plugin, the field will be left null but that should be OK because it has @CheckForNull.

[falkena]

Seems to be ok: https://stackoverflow.com/questions/16261383/delete-field-from-old-java-class-implementing-serializable/16261562

[KalleOlaviNiemitalo]

This override might not be necessary at all. It used to ensure that both repositoryType and cloneLinks were set. However, you deleted repositoryType, and cloneLinks is only used in build(SCMHead head, SCMRevision revision), which itself gets the clone links from Bitbucket if not already set. Before your changes, the override might have been necessary because some methods that read repositoryType did not expect null there.

I'm not sure whether deleting the override entirely breaks binary compatibility in Java. If it does, then keep the override and just call super from there.

Or is this necessary for scenarios where a previously initialised cloneLinks list is now out of date — because the SSH port number of the Bitbucket server has been changed, or because build(SCMHead head, SCMRevision revision) was not able to query the links and made incorrect assumptions? Perhaps the Jenkins administrator used to be able to recover from that situation by saving the project configuration again in Jenkins. That seems unlikely to have worked, though, because repositoryType would usually have been non-null and prevented afterSave from making the request.

[falkena]

As far i understood, this method will be called after job saving. Previous code fetched links, if new job was created, The logic behind was: There is new job available, repository type was changed --> fetch links, if possible. If already existing job was modified, then no action was performed. Now it's the same logic, excepting the guard is cloneLinks and not repositoryType. The question now is: Shall the links be fetched again, if job was changed. From my point of view, yes. I would implement it as:

try {
    BitbucketRepository r = buildBitbucketClient().getRepository();
    Map<String, List<BitbucketHref>> links = r.getLinks();
    if (links != null && links.containsKey("clone")) {
        cloneLinks = links.get("clone");
    }
} catch (InterruptedException | IOException e) {
    LOGGER.log(Level.SEVERE,
    "Could not determine clone links of " + getRepoOwner() + "/" + getRepository() +
    " on " + getServerUrl() + " for " + getOwner() + " falling back to generated links", e);
}

i.e. without if. FDYT?

[KalleOlaviNiemitalo]

Likewise, I don't think this is necessary. In previous versions, if the plugin made an HTTP request to get the repository type and got the clone links for free in the same response, it made sense to save them and not have to make a separate request later.

[falkena]

Removing this will lead in changed logic. If some error during link fetching is occured, then exception will be thrown and the code below will not be executed. Sadly, i've no idea, if this was implemented on purpose or not. I've no strong preference about leave or remove.

[KalleOlaviNiemitalo]

Fine then. If a user installs a new version of the plugin and the results are serialised without repositoryType, and the user then downgrades the plugin, the field will be left null but that should be OK because it has @CheckForNull.