RFR: 2222: Remove JbsVault

[Erik Joelsson]

On Wed, 3 Apr 2024 19:47:35 GMT, Erik Duveblad <ehelin at openjdk.org> wrote:

> Hi all,
> 
> please review this patch that removes the class `JbsVault`. `JbsVault` is a duplicate of `JiraVault` and was once added because there was no way to create a backport issue outside of the `org.openjdk.skara.issuetracker` module (the interface `IssueProject` does not contain any functionality for creating a backport). So the `JIraVault` was copied to `JbsVault` (which was in the `org.openjdk.skara.bots.notify` module).
> 
> This patch instead tries to make it explicit that an `IssueTracker` can support "extensions" in the form of custom REST endpoints. Code outside of `org.openjdk.skara.issuetracker` can query an `IssueTracker` to check if it supports a particular endpoint. While the approach is nice (and much better than `JbsVault`) there is still some knowledge left in `JiraHost` about backports: this is because of the field `defaultSecurity`. The `security` property is a JBS extension to JIRA and shouldn't really be in `JiraHost`. I have a draft patch that adds support for custom properties to an issue tracker, but that patch is huge (and not finished) and I would like to get this one integrated first.
> 
> I added support for a "backport endpoint" to `TestHost` as well so now all the unit tests are using the same code path as the production code (which means we can remove the method `createBackportIssue` from `JbsBackport`). I also added an integration test for `JbsBackport` to ensure that the endpoint works.
> 
> Thanks,
> Erik
> 
> ### Testing
> - [x] Added an integration test for REST endpoint used
> - [x] Added unit test support and all unit tests pass

This is a nice refactorization. Getting rid of the JbsVault is very appreciated. Approving this with a comment about a comment. Please fix that if you agree.

issuetracker/src/main/java/org/openjdk/skara/issuetracker/jira/JiraIssueTrackerFactory.java line 53:

> 51:             } else {
> 52:                 if (credential.username().isEmpty() && !credential.password().isEmpty()) {
> 53:                     return createWithPat(uri, credential.password());

The comment on this method needs to be updated as it's now called from non test code. Even if we are only using it for tests in practice, that isn't enforced anywhere, so the current comment is misleading.

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

Marked as reviewed by erikj (Lead).

PR Review: https://git.openjdk.org/skara/pull/1628#pullrequestreview-1978260747
PR Review Comment: https://git.openjdk.org/skara/pull/1628#discussion_r1550625517