Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Board Review: Service Versions #1043

Closed
heaths opened this issue Feb 25, 2020 · 7 comments
Closed

Board Review: Service Versions #1043

heaths opened this issue Feb 25, 2020 · 7 comments
Assignees
Labels
architecture board-review Request for an Architectural Board Review

Comments

@heaths
Copy link
Member

heaths commented Feb 25, 2020

Storage has already started supporting a new stable service version, while Key Vault is about to add support for a preview service version. We have many questions and would like to discuss and arrive at some guidance how Azure SDK should deal with service versions.

Scenario

What's driving this is that Key Vault is adding a 7.1-preview service version. This will likely be supported for some time before it GAs, and perhaps longer after that depending on service guidance. In response, we want to release a 4.1-preview client version that contains the new APIs they need to preview to customers.

Client version options

There are a couple of ways to do this, both with semvers or date versions:

  1. ClientOptions.ServiceVersion enumerations can define, for example, a V7_1_Preview version that equates to "7.1-preview". When the service GAs, we'd keep that version and add a V7_1 version when we release new packages. We could never drop V7_1_Preview even if the service eventually drops support.
  2. ClientOptions.ServiceVersion enumerations can define, for example, a V7_1 version that equates to "7.1-preview" and change that when the service GAs and we release new packages.

On a related note, does this necessitate the client being in a preview state in directly correlation to service being in a preview state? If it does, because the client is in a preview state can we drop the V7_1_Preview service version before we GA and after the service has GA'd?

Default client version

I can find no guidance on what the default service version should be. Key Vault currently defaults to "7.0" - our track 2 "RTM", if you will. It does define a constant LatestVersion value, however. Storage, on the other hand, defaults to the latest version.

  1. On one hand, defaulting to the latest helps push clients to the latest service version and eventually the service could deprecate older versions. Especially during previews, this can help uncover bugs in both the service and client.
  2. On the other hand, this could mean customers run into unexpected breaking service changes. As with typical libraries, devs don't like to update unless they have to. Is it their responsibility to pin to a particular version via ClientOptions, or should we do it for them through a fixed default or, perhaps, upgrade periodically and intentionally?

What should guidance be in this case?

Testing different service versions

The guidelines state that all versions should be tested and recorded. While ideal, we also currently have a considerable amount of space consumed by recorded tests. Should we follow the guidelines, or - for now - try to mitigate growing repo sizes and test a single version when possible? If so, should we test the default version (or higher, as needed for some methods or models) as decided above?

Inconsistencies

There are also inconsistencies throughout the languages, as pointed out by @jongio:

.NET

  • Supports multiple service versions passed to clients.
  • Hardcoded to use a XXClient.ServiceVersion enumeration - does not allow for user-provided string.
  • SDK needs to release a new version to support new service versions.
  • Key Vault defaults to "7.0" instead of LatestVersion.

TypeScript/JavaScript

Python

  • Defaults to latest supported version
  • Allows override via string in Mixin

Java

  • Supported via XXServiceVersion enum.
  • Accepts string - user can provide any version.
  • Get latest version via XXServiceVersion.getLatest() static method.
@tg-msft
Copy link
Member

tg-msft commented Feb 26, 2020

You can take a quick look at Azure/azure-sdk-for-net#9582 to see how we're testing multiple versions of Storage.

@tg-msft
Copy link
Member

tg-msft commented Feb 26, 2020

The C# Guidelines weigh on the default version:

The version parameter must be required, and default to the latest supported service version.

But it doesn't draw a distinction between Previews and Stable. I'm not sure if the other languages are any more explicit.

I would imagine we'd ship

  • a GA version of KeyVault v4.X.0 with Latest meaning "Latest Stable release" and
  • a Preview version of KeyVault v4.X.0-Preview1 with Latest meaning "Latest Preview release"

@adrianhall
Copy link
Member

Scheduled for Mar-2

@heaths
Copy link
Member Author

heaths commented Feb 26, 2020

That distinction is important, and any guidelines on which version to use should be general. To be honest, I didn't even check the C# guidelines because I expected general guidelines for anything related to this. JS Key Vault, for example, is making a distinction between Preview and Stable.

But regarding service version specific, one lingering question is whether we should explicitly define constants/values (and in some languages, there is no distinction like JS) for "7.1" mapping to "7.1-preview" until it ships, or just "7.1-preview" and keep it forever even if the service doesn't. Or should we keep our packages preview until a service GAs then drop "-preview" and GA our packages? I imagine for many client packages they may want to preview some functionality while keeping the package GA, though I'm not sure if that's allowed either. That's one thing we need guidance on as well.

@jongio can you also distill the info you sent me offline and update this thread this week so people have time to grok it before our meeting on March 2nd?

@heaths
Copy link
Member Author

heaths commented Mar 2, 2020

I updated the original description with some information @jongio sent me offline.

@adrianhall
Copy link
Member

adrianhall commented Mar 2, 2020

Notes from architecture board:

Recording: https://msit.microsoftstream.com/video/1cb87c26-f4f7-4c98-b953-cea00c451b0d

  • There are no general guidelines for service API versioning. Should they be written?
    • We should certainly talk about this!
    • No good guidance; consensus seems to be take the latest version.
  • Proposal:
    • GA libraries should default to latest stable version of the service API
  • Java doesn't have EditorBrowsable=Never (IDE support for hiding)
  • What about if the service team changes their mind around what the version is?
  • @johanste - preview libraries should still point to latest stable by default - opt in to preview features
    • Less agreement around this point.
    • We don't have a good way to preview "pieces" of an API
    • We don't want to introduce complexity. However, SLA guarantees from service team are associated with GA, not preview.
  • Are we fixing a real problem or just adding more flexibility?
  • Cosmos provides a capability to opt-in to something you want to try out (dangerous, but we don't have a better answer). It's opt in and not documented.
  • Preview Swaggers can change - stable Swaggers mean the API contract is already set in stone.
  • Aside from @johanste , no-one thinks doing preview features in GA packages is a good idea. JS isn't going to "die on the hill".
  • The primary use case here is customer wants to use 99% GA features, but include 1 preview feature that is preview on the service + client library.
  • Should we consider configuration for a service version across an application? Or "use this profile"
  • What happens when a service API version is deprecated? We should be able to remove the enum value eventually? It's a sliding window of support. (Remove / throw an exception / log a warning / etc.)

ACTION: Adrian will submit a PR to add the following requirements to general guidelines

  • GA libraries MUST target GA service API versions.
  • LATEST in GA library MUST point to the latest GA service API version.
  • Public preview versions of the library MUST point to latest public preview service API version.
  • Client libraries MUST include all versions of the service API that the client library supports in a ServiceVersion enum.
  • Client libraries MUST document what "latest service API version" points to.
  • ServiceVersion enum values MUST match the service version in swagger.
  • If a service API version is preview, then the ServiceVersion enum should be suffixed by -preview.
  • Given a X.X-Preview (service API version) that will transition to X.X in GA, accept the X.X in the client library and add the -Preview "under the covers" (then remove that code after the GA version is released)

Recorded Tests:

  • .NET and Java are both testing PRs with recorded tests against latest version.

ACTION:

  • Replace "DO recreate recorded tests for a specific service version" with "DO create recorded tests for the latest service version"

Inconsistencies:

  • Should Python/JS use a common moniker (service version) for specifying the service versions.
  • Java and .NET feel like differences in idiomatic usage.
  • Primarily, we are concerned with "when a user calls support and needs to know the API version..."
  • A lot of differing version features should require documentation on supported versions.

ACTION:

  • Python & JavaScript to come up with mechanism for providing supported service API versions to the customer.

kyle-patterson pushed a commit that referenced this issue Mar 25, 2020
* (#1043) Updates to guidelines from arch.board meeting

* Apply suggestions from code review

Updates from @bterlson

Co-Authored-By: Brian Terlson <brian.terlson@microsoft.com>

Co-authored-by: Adrian Hall <adrian.hall@microsoft.com>
Co-authored-by: Brian Terlson <brian.terlson@microsoft.com>
@kyle-patterson
Copy link
Member

Closing out as the guideline changes have been completed.

bterlson added a commit to bterlson/azure-sdk that referenced this issue Apr 16, 2020
* (Azure#1043) Updates to guidelines from arch.board meeting

* Apply suggestions from code review

Updates from @bterlson

Co-Authored-By: Brian Terlson <brian.terlson@microsoft.com>

Co-authored-by: Adrian Hall <adrian.hall@microsoft.com>
Co-authored-by: Brian Terlson <brian.terlson@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture board-review Request for an Architectural Board Review
Projects
None yet
Development

No branches or pull requests

4 participants