Skip to content

Conversation

iliana
Copy link
Contributor

@iliana iliana commented Aug 6, 2025

Part of #8727 (the easy ones)

@iliana iliana requested a review from davepacheco August 6, 2025 21:20
Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect to see changes for three progenitor client packages that aren't here:

  • ntp-admin
  • sled-agent
  • oximeter

SupportedVersion, SupportedVersions, api_versions,
};

api_versions!([
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had talked about versioning the three Clickhouse services separately in separate modules. Did you opt not to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I added a note in this block (just below here) suggesting that whoever finds a need to add a new version to these APIs should determine whether all the services should share the same versions, or whether to split them into modules. Although now a couple of days later I'm questioning my choices.

@davepacheco
Copy link
Collaborator

davepacheco commented Aug 8, 2025

We also need to update the server implementations to actually process the version field. I think that's not critical for R17 but if you don't do it now, could you file a new issue for that and make sure it gets the "important non-blocker" status? (this is in the "deliverable" field of the Reconfigurator project)

Edit: to be clear, that's a change like this:

.version_policy(dropshot::VersionPolicy::Dynamic(Box::new(
dropshot::ClientSpecifiesVersionInHeader::new(
"api-version"
.parse::<http::header::HeaderName>()
.expect("api-version is a valid header name"),
dns_server_api::VERSION_SOA_AND_NS,
),
)))

If it's easy, that would be nice to do now because it'll be easy to forget later and that will break things.

Copy link
Collaborator

@davepacheco davepacheco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Some test failures, though.

@iliana
Copy link
Contributor Author

iliana commented Aug 19, 2025

The test failures are due to the gateway tests not sending an api-version header in any of its requests, which is difficult to do with dropshot::test_util::ClientTestContext. I'm going to back out the gateway API versioning in this PR and add it in a future PR which refactors the tests.

@iliana iliana merged commit a582b52 into main Aug 19, 2025
18 checks passed
@iliana iliana deleted the iliana/api-versioning branch August 19, 2025 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants