RFR: 1910: Basic support for Bitbucket as a Forge [v2]

[Erik Helin]

On Fri, 12 May 2023 23:00:41 GMT, Erik Joelsson <erikj at openjdk.org> wrote:

>> This patch adds very basic support for Bitbucket as a Forge, just enough to be able to use a Bitbucket repo as the source for mirroring.
>> 
>> In order to support ssh for bitbucket, I moved the `configureSshKey` method from `GitLabForgeFactory` to a new utility class, so that it could be shared more easily.
>
> Erik Joelsson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments

A general comment regarding `ForgeUtils.configureSshKey`: I know this existing code, but I think a cleaner (and safer) solution would have been to stored the configuration in a separate file per host and then use `ssh -F /path/to/hosts/ssh/config/file`. Not worth thinking in this patch, but could be something for the future.

Otherwise just stylistic nits and minor style suggestions. Overall nice work and cool to see it how easy it was to add a minimal new forge!

forge/src/main/java/org/openjdk/skara/forge/bitbucket/BitbucketForgeFactory.java line 59:

> 57:             sshport = configuration.get("sshport").asInt();
> 58:         }
> 59:         if (credential != null) {

I would probably have used an `ssh` object in the config here, as in:


if (configuration != null && configuration.contains("ssh")) {
    var ssh = configuration.get("ssh");
    var key = ssh.get("key").asString();
    var port = ssh.contains("port") ? ssh.get("port").asInt() : 22;
}

forge/src/main/java/org/openjdk/skara/forge/bitbucket/BitbucketHost.java line 55:

> 53:         this.sshPort = sshPort;
> 54:         this.credential = null;
> 55:     }

It seems like credential cannot be `null` when `useSsh` is `true` (see the method `sshHostString`). Maybe the constructors should reflect this? Or use static factory methods like `BitBucketHost.withSSH` and/or `BitBucketHost.withPAT`?

forge/src/main/java/org/openjdk/skara/forge/bitbucket/BitbucketRepository.java line 51:

> 49: 
> 50: public class BitbucketRepository implements HostedRepository {
> 51:     private final BitbucketHost bitbucketHost;

I would have named the field just `host`.

forge/src/main/java/org/openjdk/skara/forge/bitbucket/BitbucketRepository.java line 52:

> 50: public class BitbucketRepository implements HostedRepository {
> 51:     private final BitbucketHost bitbucketHost;
> 52:     private final String repositoryName;

And here I would have just used the field `name` 😄

-------------

Marked as reviewed by ehelin (Reviewer).

PR Review: https://git.openjdk.org/skara/pull/1521#pullrequestreview-1429220805
PR Review Comment: https://git.openjdk.org/skara/pull/1521#discussion_r1195575871
PR Review Comment: https://git.openjdk.org/skara/pull/1521#discussion_r1195578593
PR Review Comment: https://git.openjdk.org/skara/pull/1521#discussion_r1195579327
PR Review Comment: https://git.openjdk.org/skara/pull/1521#discussion_r1195579689