(Thinking aloud) maybe we could return a wrapper structure with something like

type SupportedMicroversions struct {
    MaxMajor int
    MaxMinor int
    MinMajor int
    MinMinor int
}

Then some of these functions become methods on it, like

func (vs SupportedMicroversions) Require(version string) error
func (vs SupportedMicroversions) IsSupported(version string) bool
func (vs SupportedMicroversions) Choose(versions []string) string

etc

Also: you want to pass the client as a pointer, not by value.

Would the idea be to hold on to this wrapper and pass it around? I'm worried it would become hard to use. If I need to check/require microversions in many different places in a code base it would probably be cumbersome. Alternatively, I would end up calling GetSupportedMicroversions all over the place.

I only thing it will be more convenient to use methods on the wrapper rather than passing two strings around and parsing them each time. There is also a risk of people using the wrong order (what goes first, min or max)?

Ah good point! I was still in my head thinking about how I did it in CAPO, where the min and max is stored in the ComputeClient and then these functions become methods on that. Maybe I can find some way to integrate this better with the ServiceClient 🤔

You're sure this is limited to "identity service", i.e. Keystone?

It has a reference to IdentityBase down the code, dunno why.

Yes, I got fooled by the doc string into believing this could be used for Nova. I was very confused about why it never could find a good version to use until @mdbooth pointed out that it is taking the ProviderClient as input, not the ServiceClient. It is indeed only checking the identity endpoint and picks a version for Keystone.

Given that gophercloud is working on a major release, maybe it's time to change that?

Yeah that would be nice!

It would also be really good to have this in the v1 branch though, if possible. We have a bunch of potential near term use cases for it, one of which is described in kubernetes-sigs/cluster-api-provider-openstack#1715 (comment).

@mdbooth I thought you did not want this kind of negotiation of the version? I have reworked the CAPO PR for that reason to set an explicit version in all cases instead of using a function like this. In which cases do you think we should use this?

I probably got confused and thought I was commenting on the PR in general :)

You are correct, I don't want to do negotiation in CAPO: we either get a specific version or we don't execute the code. There may be cases where some code might run with one of a small number of specific microversions (e.g. GET server) but we should never, e.g. request microversion N and get microversion N+1.

That said, it might be useful to be able to specify a microversion limit for testing so we can test old microversions against a newer server. The microversion requested would still be at the point of use, though, and we would still either get it or not get it.

However, CAPO hopefully isn't the only consumer of this code. Negotiation might be useful elsewhere, e.g. a CLI utility.

nit: I'd also love to see

func (ms SupportedMicroversions) Choose(options []string) string

The internal properties of this struct (in particular: the fact that they're split into exactly major and minor) look to me like implementation details. Please consider un-exporting the properties (and expose getters and setters as necessary) to reduce the API surface of our compatibility promise.

"the fact that they're split into exactly major and minor" is the hard fact that is embedded into the definition of microversions. I'm fine with hiding private details as long as I can still access them via accessors (not just the string representations - actual numbers).

Reference: https://specs.openstack.org/openstack/api-sig/guidelines/microversion_specification.html

The returned type (SupportedMicroversions) contains a minimum and a maximum version. Are we sure that this is sufficient data to know what version is supported?

For example: consider the hypothetical situation in which service X on OSP Stegosaurus exposes microversions from 1.0 to 1.2 and from 2.0 to 2.3. A security issue is found that is so bad, it needs backport. And it also requires a slight change in the API. So after the fix, service X on OSP T-Rex publishes microversions 1.3 and 2.4. Knowing minimum 1.0 and maximum 2.4 is not a guarantee that 1.3 is available (unless you run more elaborate checks).

Your example is too hypothetical :) First, it assumes having two major versions on one endpoint, which is not how things tend to be done. Second, it assumes two major versions with microversions which is a thing that is quite unlikely to happen because the very idea of microversions implies that you can do breaking changes without creating major versions. We do ponder Ironic v2 from time to time, but that's more of a wishful thinking (file it under "rewrite Ironic in Go").

The way I see it, the assumption is to think that my example won’t happen. Which may or may not be a safe assumption.

It's a question of whether you want to try covering 100% of potential future cases. It's not a terrible idea if you do, but that also mean you will do much more work than you need to. With rust-openstack, I took the approach of "I'll think about it when it happens" (fwiw, openstacksdk does the same), which has its own pros and cons.

From what I can see, this (min and max) is the only information we can get from a single endpoint so there is nothing more we can do.

I wonder if we can assume that all services will either use version, versions, or no versions. What if we have service-specific logic instead? Have you considered having service-specific logic, maybe exposed in the ServiceClient through an interface?

In theory - no. In practice, everyone just copied what Nova did :)

As a data point, I've implemented a similar thing in rust-osauth long ago, and nobody complained so far: https://github.com/dtantsur/rust-osauth/blob/master/src/protocol.rs#L31-L39

How is Swift versioning its API?

I don't think it actually does. Of the core services, I think only Nova, Keystone and Cinder (and Ironic if you count it core) use microversioning.

I like this function signature. You give me a version, and I tell you if it's OK. This requires many less assumptions. Can we make it so that this is the only new exported symbol, and unexport all the rest?

Please don't. GopherCloud is already bare bones enough to justify another high-level client built upon it.

I may be failing to see your intended use case. Do you want to share the one you have one in mind?

The use case I’m most inclined to think: a client has two, max three behaviours coded for a specific action, each corresponding to a specific API microversion. The client would ask for its preferred version, then if not available it would test the fallback behaviour, and so on. In this case the client knows what exact microversion it needs and asks Goohercloud whether it is available. Do you have a case where a client needs something different, like a range?

Largely, I want to know in advance what features I can and cannot use, so that I don't need to delay this decision until much later in the code. Our code is organized in the way that the controller layer does not know about the Ironic implementation details, and we'd rather keep it that way.