Skip to content

Commit eb9911c

Browse files
committed
feat(adbc): add --version and require the version to be installed
The adbc commands were pinned to the active version, unlike `ampup install` which takes one. Add --version to install, list, and uninstall. Resolving the version now also requires it to be installed. Installing amp replaces the whole version directory, so drivers placed under a version whose binaries are not there yet would be destroyed by the next `ampup install`. Part of edgeandnode/amp#2600.
1 parent 6569282 commit eb9911c

3 files changed

Lines changed: 117 additions & 29 deletions

File tree

ampup/src/commands/adbc.rs

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -19,37 +19,35 @@ use crate::{
1919
version_manager::VersionManager,
2020
};
2121

22-
/// Install an ADBC driver for the active amp version.
22+
/// Install an ADBC driver for an amp version (the active one by default).
2323
pub async fn install(
2424
driver: &str,
2525
install_dir: Option<PathBuf>,
2626
repo: String,
2727
github_token: Option<String>,
2828
arch: Option<String>,
2929
platform: Option<String>,
30+
version: Option<String>,
3031
) -> Result<()> {
3132
let driver = parse_driver(driver)?;
3233
let config = Config::new(install_dir)?;
3334
let github = GitHubClient::new(repo, token::resolve_github_token(github_token))?;
34-
install_driver(&github, config, driver, arch, platform).await
35+
install_driver(&github, config, driver, arch, platform, version).await
3536
}
3637

37-
/// Fetch, verify, extract, and place `driver` for the active amp version using
38-
/// an already-constructed GitHub client. Split from [`install`] so tests can
38+
/// Fetch, verify, extract, and place `driver` for an amp version using an
39+
/// already-constructed GitHub client. Split from [`install`] so tests can
3940
/// inject a client pointed at a mock server.
4041
pub(crate) async fn install_driver(
4142
github: &GitHubClient,
4243
config: Config,
4344
driver: Driver,
4445
arch: Option<String>,
4546
platform: Option<String>,
47+
version: Option<String>,
4648
) -> Result<()> {
4749
let version_manager = VersionManager::new(config);
48-
49-
// Drivers are pinned to an installed amp version, so one must be active.
50-
let version = version_manager
51-
.get_current()?
52-
.ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?;
50+
let version = resolve_version(&version_manager, version)?;
5351

5452
let platform = resolve_platform(platform)?;
5553
let arch = resolve_arch(arch)?;
@@ -130,15 +128,18 @@ fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()>
130128
Ok(())
131129
}
132130

133-
/// List installed ADBC drivers for the active amp version.
134-
pub fn list(install_dir: Option<PathBuf>) -> Result<()> {
131+
/// List installed ADBC drivers for an amp version (the active one by default).
132+
pub fn list(install_dir: Option<PathBuf>, version: Option<String>) -> Result<()> {
135133
let config = Config::new(install_dir)?;
136134
let version_manager = VersionManager::new(config);
137135

138-
let Some(version) = version_manager.get_current()? else {
136+
// Without an explicit version, having none active is an empty state rather
137+
// than an error.
138+
if version.is_none() && version_manager.get_current()?.is_none() {
139139
ui::info!("No active amp version");
140140
return Ok(());
141-
};
141+
}
142+
let version = resolve_version(&version_manager, version)?;
142143

143144
let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?;
144145
if drivers.is_empty() {
@@ -156,16 +157,17 @@ pub fn list(install_dir: Option<PathBuf>) -> Result<()> {
156157
Ok(())
157158
}
158159

159-
/// Uninstall an ADBC driver from the active amp version.
160-
pub fn uninstall(install_dir: Option<PathBuf>, driver: &str) -> Result<()> {
160+
/// Uninstall an ADBC driver from an amp version (the active one by default).
161+
pub fn uninstall(
162+
install_dir: Option<PathBuf>,
163+
driver: &str,
164+
version: Option<String>,
165+
) -> Result<()> {
161166
let driver = parse_driver(driver)?;
162167

163168
let config = Config::new(install_dir)?;
164169
let version_manager = VersionManager::new(config);
165-
166-
let version = version_manager
167-
.get_current()?
168-
.ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?;
170+
let version = resolve_version(&version_manager, version)?;
169171

170172
let driver_dir = version_manager
171173
.config()
@@ -190,6 +192,28 @@ pub fn uninstall(install_dir: Option<PathBuf>, driver: &str) -> Result<()> {
190192
Ok(())
191193
}
192194

195+
/// Resolve the amp version to operate on: an explicit one, or the active one.
196+
///
197+
/// The version must already be installed. Installing amp replaces the whole
198+
/// version directory, so drivers placed under a version whose binaries are not
199+
/// there yet would be destroyed by the next `ampup install`.
200+
fn resolve_version(version_manager: &VersionManager, version: Option<String>) -> Result<String> {
201+
let version = match version {
202+
Some(version) => version,
203+
None => version_manager
204+
.get_current()?
205+
.ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?,
206+
};
207+
208+
if !version_manager.is_installed(&version) {
209+
bail!(
210+
"amp {} is not installed; run `ampup install {version}` first",
211+
ui::version(&version),
212+
);
213+
}
214+
Ok(version)
215+
}
216+
193217
/// The catalog drivers currently installed under `drivers_dir`.
194218
///
195219
/// Only complete installs count: an entry must be a directory named after a

ampup/src/main.rs

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ enum SelfCommands {
185185

186186
#[derive(Debug, clap::Subcommand)]
187187
enum AdbcCommands {
188-
/// Install an ADBC driver for the active amp version
188+
/// Install an ADBC driver for an amp version
189189
Install {
190190
/// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp)
191191
#[arg(long, env = "AMP_DIR")]
@@ -209,23 +209,35 @@ enum AdbcCommands {
209209
/// Override platform detection (linux, darwin)
210210
#[arg(long)]
211211
platform: Option<String>,
212+
213+
/// Amp version to install the driver for (defaults to the active one)
214+
#[arg(long = "version")]
215+
amp_version: Option<String>,
212216
},
213217

214-
/// List installed ADBC drivers for the active version
218+
/// List installed ADBC drivers for an amp version
215219
List {
216220
/// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp)
217221
#[arg(long, env = "AMP_DIR")]
218222
install_dir: Option<std::path::PathBuf>,
223+
224+
/// Amp version to list drivers for (defaults to the active one)
225+
#[arg(long = "version")]
226+
amp_version: Option<String>,
219227
},
220228

221-
/// Uninstall an ADBC driver
229+
/// Uninstall an ADBC driver from an amp version
222230
Uninstall {
223231
/// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp)
224232
#[arg(long, env = "AMP_DIR")]
225233
install_dir: Option<std::path::PathBuf>,
226234

227235
/// Driver to uninstall (e.g. postgresql)
228236
driver: String,
237+
238+
/// Amp version to uninstall the driver from (defaults to the active one)
239+
#[arg(long = "version")]
240+
amp_version: Option<String>,
229241
},
230242
}
231243

@@ -326,18 +338,31 @@ async fn run() -> anyhow::Result<()> {
326338
github_token,
327339
arch,
328340
platform,
341+
amp_version,
329342
} => {
330-
commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform)
331-
.await?;
343+
commands::adbc::install(
344+
&driver,
345+
install_dir,
346+
repo,
347+
github_token,
348+
arch,
349+
platform,
350+
amp_version,
351+
)
352+
.await?;
332353
}
333-
AdbcCommands::List { install_dir } => {
334-
commands::adbc::list(install_dir)?;
354+
AdbcCommands::List {
355+
install_dir,
356+
amp_version,
357+
} => {
358+
commands::adbc::list(install_dir, amp_version)?;
335359
}
336360
AdbcCommands::Uninstall {
337361
install_dir,
338362
driver,
363+
amp_version,
339364
} => {
340-
commands::adbc::uninstall(install_dir, &driver)?;
365+
commands::adbc::uninstall(install_dir, &driver, amp_version)?;
341366
}
342367
},
343368
None => {

ampup/src/tests/it_adbc.rs

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@ use crate::{
1515
commands::adbc,
1616
config::Config,
1717
github::GitHubClient,
18-
tests::{fixtures::TempInstallDir, mock_github},
18+
tests::{
19+
fixtures::{MockBinary, TempInstallDir},
20+
mock_github,
21+
},
1922
};
2023

2124
const LIB: &str = "libadbc_driver_postgresql.so";
@@ -45,6 +48,7 @@ async fn adbc_install_places_driver_and_manifest() {
4548
let temp = TempInstallDir::new().expect("temp install dir");
4649
let version = "v1.0.0";
4750
fs::write(temp.current_version_file(), version).expect("write active version");
51+
MockBinary::create(&temp, version).expect("install version binaries");
4852

4953
// The release asset the installer will fetch, plus its advertised digest.
5054
let tarball = make_targz(&[
@@ -75,6 +79,7 @@ async fn adbc_install_places_driver_and_manifest() {
7579
Driver::Postgresql,
7680
Some("x86_64".to_string()),
7781
Some("linux".to_string()),
82+
None,
7883
)
7984
.await
8085
.expect("install should succeed");
@@ -102,6 +107,7 @@ async fn adbc_install_rejects_asset_without_digest() {
102107
let temp = TempInstallDir::new().expect("temp install dir");
103108
let version = "v1.0.0";
104109
fs::write(temp.current_version_file(), version).expect("write active version");
110+
MockBinary::create(&temp, version).expect("install version binaries");
105111

106112
let tarball = make_targz(&[
107113
(LIB, b"ELF"),
@@ -130,6 +136,7 @@ async fn adbc_install_rejects_asset_without_digest() {
130136
Driver::Postgresql,
131137
Some("x86_64".to_string()),
132138
Some("linux".to_string()),
139+
None,
133140
)
134141
.await
135142
.expect_err("install should refuse an asset without a digest");
@@ -141,19 +148,51 @@ async fn adbc_install_rejects_asset_without_digest() {
141148
);
142149
}
143150

151+
#[tokio::test]
152+
async fn adbc_install_refuses_a_version_that_is_not_installed() {
153+
let temp = TempInstallDir::new().expect("temp install dir");
154+
// v1.0.0 is active and installed; v2.0.0 has no binaries.
155+
fs::write(temp.current_version_file(), "v1.0.0").expect("write active version");
156+
MockBinary::create(&temp, "v1.0.0").expect("install version binaries");
157+
158+
// No mock server: resolving the version fails before anything is fetched.
159+
let github = GitHubClient::with_api_base("http://127.0.0.1:1".to_string()).expect("client");
160+
let config = Config::new(Some(temp.path().to_path_buf())).expect("config");
161+
162+
let err = adbc::install_driver(
163+
&github,
164+
config,
165+
Driver::Postgresql,
166+
Some("x86_64".to_string()),
167+
Some("linux".to_string()),
168+
Some("v2.0.0".to_string()),
169+
)
170+
.await
171+
.expect_err("install should refuse a version that is not installed");
172+
assert!(err.to_string().contains("is not installed"), "got: {err}");
173+
174+
// Installing amp replaces the whole version directory, so drivers must not
175+
// be placed under a version whose binaries are not there yet.
176+
assert!(
177+
!temp.versions_dir().join("v2.0.0").exists(),
178+
"no version directory should be created for an uninstalled version",
179+
);
180+
}
181+
144182
#[test]
145183
fn adbc_uninstall_removes_installed_driver() {
146184
let temp = TempInstallDir::new().expect("temp install dir");
147185
let version = "v1.0.0";
148186
fs::write(temp.current_version_file(), version).expect("write active version");
187+
MockBinary::create(&temp, version).expect("install version binaries");
149188

150189
let drivers_dir = temp.versions_dir().join(version).join("drivers");
151190
let driver_dir = drivers_dir.join("postgresql");
152191
fs::create_dir_all(&driver_dir).expect("driver dir");
153192
fs::write(driver_dir.join(LIB), b"ELF").expect("lib");
154193
fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest");
155194

156-
adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql")
195+
adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None)
157196
.expect("uninstall should succeed");
158197

159198
assert!(!driver_dir.exists(), "driver directory removed");

0 commit comments

Comments
 (0)