Replace get_keyring_service by KEYRING_SERVICE class variable

[Lotram]

First commit

Fix a test in test_service.py : since this commit the test check each abstract method is defined only once, which does not really make sense. I changed it back to check the number of calls, while adding an allowlist for some abstract methods we do want to call from concrete methods.

An alternative could be to simply remove this test, since we already have an exception

Second commit

Replace Service.get_keyring_service by a ServiceConfig.KEYRING_SERVICE string

[ryneeverett]

Regarding the first commit -- oops! Thanks for catching this.

It isn't clear to me that an exception is justified for get_keyring_service though. Perhaps this would be a good time to eliminate the method in favor of a class property string on which Service.get_secret calls .format(config)?

[Lotram]

I'm ok with removing get_keyring_service, but it wouldn't work right away with KanboardService and PhabricatorService:

    def get_keyring_service(config: KanboardConfig) -> str:
        parsed = urlparse(config.url)
        return f"kanboard://{config.username}@{parsed.netloc}"

One solution would be to use a computed_field for the host, in both KanboardConfigandPhabricatorConfig`

Another solution would be to make get_keyring_service a method of the ServiceConfig subclasses, instead of the Service ones ? IMO it makes sense to attach this to the config. Users could want to customize this in their config, if we ever want to support this.

[ryneeverett]

The computed_field solution seems good. Making this a string rather than a method is the main thing I'd like to see.

I'd be ok with that string moving to ServiceConfig, but I wouldn't want to see the get_keyring_service method moved to ServiceConfig. As you probably realize, it shouldn't be configurable at this point since there is no apparent demand for that.

[ryneeverett]

This is a breaking change to the service API so we need to bump the version to 2.0 (LATEST_API_VERSION, API_VERSION of each service, header of api.rst). I'd also like to see backwards compatibility:

    @property
    def keyring_service(self) -> str:
        service = get_service(self.service).API_VERSION
        if service.API_VERSION < 2:
            return service.get_keyring_service(self)
        return self.KEYRING_SERVICE.format(**self.model_dump())

[ryneeverett]

Can we move this method down below all the class variables with the rest of the methods?

[ryneeverett]

Let's remove this unnecessary comment which isn't quite accurate any more after this change.

Suggested change

# strictly required