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

fix(turborepo): Adopt std::cell::OnceCell #5763

Merged
merged 6 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions crates/turborepo-lib/src/commands/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,10 @@ fn add_space_id_to_turbo_json(base: &CommandBase, space_id: &str) -> Result<()>

#[cfg(test)]
mod test {
use std::fs;
use std::{cell::OnceCell, fs};

use anyhow::Result;
use tempfile::{NamedTempFile, TempDir};
use tokio::sync::OnceCell;
use turbopath::AbsoluteSystemPathBuf;
use turborepo_ui::UI;
use vercel_api_mock::start_test_server;
Expand Down
3 changes: 1 addition & 2 deletions crates/turborepo-lib/src/commands/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,12 +298,11 @@ async fn run_sso_one_shot_server(

#[cfg(test)]
mod test {
use std::fs;
use std::{cell::OnceCell, fs};

use reqwest::Url;
use serde::Deserialize;
use tempfile::{tempdir, NamedTempFile};
use tokio::sync::OnceCell;
use turbopath::AbsoluteSystemPathBuf;
use turborepo_ui::UI;
use vercel_api_mock::start_test_server;
Expand Down
88 changes: 37 additions & 51 deletions crates/turborepo-lib/src/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
use std::borrow::Borrow;
use std::{borrow::Borrow, cell::OnceCell};

use anyhow::Result;
use sha2::{Digest, Sha256};
use tokio::sync::OnceCell;
use turbopath::{AbsoluteSystemPath, AbsoluteSystemPathBuf};
use turborepo_api_client::APIClient;
use turborepo_ui::UI;

use crate::{
config::{
default_user_config_path, get_repo_config_path, ClientConfig, ClientConfigLoader,
RepoConfig, RepoConfigLoader, UserConfig, UserConfigLoader,
Error as ConfigError, RepoConfig, RepoConfigLoader, UserConfig, UserConfigLoader,
},
Args,
};
Expand Down Expand Up @@ -55,18 +54,26 @@ impl CommandBase {
})
}

fn create_repo_config(&self) -> Result<()> {
fn repo_config_init(&self) -> Result<RepoConfig, ConfigError> {
let repo_config_path = get_repo_config_path(self.repo_root.borrow());

let repo_config = RepoConfigLoader::new(repo_config_path)
RepoConfigLoader::new(repo_config_path)
.with_api(self.args.api.clone())
.with_login(self.args.login.clone())
.with_team_slug(self.args.team.clone())
.load()?;
.load()
}

self.repo_config.set(repo_config)?;
pub fn repo_config(&self) -> Result<&RepoConfig, ConfigError> {
self.repo_config.get_or_try_init(|| self.repo_config_init())
}

Comment on lines +67 to +69
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a rigid new pattern here across all of the config loading sources. It makes it super-clear what's happening each time.

Ok(())
pub fn repo_config_mut(&mut self) -> Result<&mut RepoConfig, ConfigError> {
// Approximates `get_mut_or_try_init`
self.repo_config
.get_or_try_init(|| self.repo_config_init())?;

nathanhammond marked this conversation as resolved.
Show resolved Hide resolved
Ok(self.repo_config.get_mut().unwrap())
}

// NOTE: This deletes the repo config file. It does *not* remove the
Expand All @@ -81,62 +88,41 @@ impl CommandBase {
Ok(())
}

fn create_user_config(&self) -> Result<()> {
let user_config = UserConfigLoader::new(default_user_config_path()?)
fn user_config_init(&self) -> Result<UserConfig, ConfigError> {
UserConfigLoader::new(default_user_config_path()?)
.with_token(self.args.token.clone())
.load()?;
self.user_config.set(user_config)?;

Ok(())
}

fn create_client_config(&self) -> Result<()> {
let client_config = ClientConfigLoader::new()
.with_remote_cache_timeout(self.args.remote_cache_timeout)
.load()?;
self.client_config.set(client_config)?;

Ok(())
.load()
}

pub fn repo_config_mut(&mut self) -> Result<&mut RepoConfig> {
if self.repo_config.get().is_none() {
self.create_repo_config()?;
}

Ok(self.repo_config.get_mut().unwrap())
}

pub fn repo_config(&self) -> Result<&RepoConfig> {
if self.repo_config.get().is_none() {
self.create_repo_config()?;
}

Ok(self.repo_config.get().unwrap())
pub fn user_config(&self) -> Result<&UserConfig, ConfigError> {
self.user_config.get_or_try_init(|| self.user_config_init())
}

pub fn user_config_mut(&mut self) -> Result<&mut UserConfig> {
if self.user_config.get().is_none() {
self.create_user_config()?;
}
pub fn user_config_mut(&mut self) -> Result<&mut UserConfig, ConfigError> {
// Approximates `get_mut_or_try_init`
self.user_config
.get_or_try_init(|| self.user_config_init())?;

nathanhammond marked this conversation as resolved.
Show resolved Hide resolved
Ok(self.user_config.get_mut().unwrap())
}

pub fn user_config(&self) -> Result<&UserConfig> {
if self.user_config.get().is_none() {
self.create_user_config()?;
}
fn client_config_init(&self) -> Result<ClientConfig, ConfigError> {
ClientConfigLoader::new()
.with_remote_cache_timeout(self.args.remote_cache_timeout)
.load()
}

Ok(self.user_config.get().unwrap())
pub fn client_config(&self) -> Result<&ClientConfig, ConfigError> {
self.client_config
.get_or_try_init(|| self.client_config_init())
}

pub fn client_config(&self) -> Result<&ClientConfig> {
if self.client_config.get().is_none() {
self.create_client_config()?;
}
pub fn client_config_mut(&mut self) -> Result<&mut ClientConfig, ConfigError> {
// Approximates `get_mut_or_try_init`
self.client_config
.get_or_try_init(|| self.client_config_init())?;

nathanhammond marked this conversation as resolved.
Show resolved Hide resolved
Ok(self.client_config.get().unwrap())
Ok(self.client_config.get_mut().unwrap())
}

pub fn args(&self) -> &Args {
Expand Down
17 changes: 7 additions & 10 deletions crates/turborepo-lib/src/config/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
use config::{Config, ConfigError, Environment};
use serde::{Deserialize, Serialize};

use crate::config::Error;

const DEFAULT_TIMEOUT: u64 = 20;

#[derive(Debug, Clone, PartialEq, Eq)]
Expand Down Expand Up @@ -49,7 +51,7 @@
self
}

pub fn load(self) -> Result<ClientConfig> {
pub fn load(self) -> Result<ClientConfig, Error> {
let Self {
remote_cache_timeout,
environment,
Expand All @@ -60,17 +62,12 @@
.add_source(Environment::with_prefix("turbo").source(environment))
.set_override_option("remote_cache_timeout", remote_cache_timeout)?
.build()?
// Deserialize is the only user-input-fallible step.
// Everything else is programmer error.
// This goes wrong when TURBO_REMOTE_CACHE_TIMEOUT can't be deserialized to u64
.try_deserialize();

// This goes wrong when TURBO_REMOTE_CACHE_TIMEOUT can't be deserialized to u64
match config_attempt {
Err(_) => Ok(ClientConfig {
config: ClientConfigValue {
remote_cache_timeout: DEFAULT_TIMEOUT,
},
}),
Ok(config) => Ok(ClientConfig { config }),
}
Comment on lines -65 to -73
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This behavior does not match any of our other configuration loaders which fail hard in the event of garbage input.

We can come back and create "this error is only a warning" behavior if we want to do so later, but for now make the failure mode consistent.

config_attempt.map(|config| ClientConfig { config })

Check failure on line 70 in crates/turborepo-lib/src/config/client.rs

View workflow job for this annotation

GitHub Actions / Build Turborepo (ubuntu, ubuntu-latest)

mismatched types

Check failure on line 70 in crates/turborepo-lib/src/config/client.rs

View workflow job for this annotation

GitHub Actions / Build Turborepo (macos, macos-latest)

mismatched types
}
}

Expand Down
1 change: 1 addition & 0 deletions crates/turborepo-lib/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![feature(error_generic_member_access)]
#![feature(provide_any)]
#![feature(option_get_or_insert_default)]
#![feature(once_cell_try)]
#![deny(clippy::all)]
// Clippy's needless mut lint is buggy: https://github.com/rust-lang/rust-clippy/issues/11299
#![allow(clippy::needless_pass_by_ref_mut)]
Expand Down
Loading