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

make RepoConfig and CommandBase use absolute system paths #4693

Merged
merged 3 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 4 additions & 0 deletions crates/turborepo-lib/src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use clap_complete::{generate, Shell};
use dunce::canonicalize as fs_canonicalize;
use serde::Serialize;
use tracing::{debug, error};
use turbopath::AbsoluteSystemPathBuf;

use crate::{
commands::{bin, daemon, link, login, logout, unlink, CommandBase},
Expand Down Expand Up @@ -489,6 +490,9 @@ pub async fn run(repo_state: Option<RepoState>) -> Result<Payload> {
current_dir()?
};

// a non-absolute repo root is a bug
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the lines above do not give an absolute path it is programmer error.

let repo_root = AbsoluteSystemPathBuf::new(repo_root).expect("repo_root is not absolute");

let version = get_version();

match cli_args.command.as_ref().unwrap() {
Expand Down
30 changes: 22 additions & 8 deletions crates/turborepo-lib/src/commands/link.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use dialoguer::{theme::ColorfulTheme, Confirm};
use dirs_next::home_dir;
#[cfg(test)]
use rand::Rng;
use turbopath::RelativeSystemPathBuf;
use turborepo_api_client::{APIClient, CachingStatus, Space, Team};

#[cfg(not(test))]
Expand Down Expand Up @@ -166,8 +167,11 @@ pub async fn link(
verify_caching_enabled(&api_client, team_id, token, Some(selected_team.clone()))
.await?;

fs::create_dir_all(base.repo_root.join(".turbo"))
.context("could not create .turbo directory")?;
fs::create_dir_all(
base.repo_root
.join_relative(RelativeSystemPathBuf::new(".turbo").expect("relative")),
arlyon marked this conversation as resolved.
Show resolved Hide resolved
)
.context("could not create .turbo directory")?;
base.repo_config_mut()?
.set_team_id(Some(team_id.to_string()))?;

Expand Down Expand Up @@ -395,7 +399,9 @@ fn enable_caching(url: &str) -> Result<()> {
}

fn add_turbo_to_gitignore(base: &CommandBase) -> Result<()> {
let gitignore_path = base.repo_root.join(".gitignore");
let gitignore_path = base
.repo_root
.join_relative(RelativeSystemPathBuf::new(".gitignore").expect("relative"));

if !gitignore_path.exists() {
let mut gitignore = File::create(gitignore_path)?;
Expand All @@ -419,7 +425,9 @@ fn add_turbo_to_gitignore(base: &CommandBase) -> Result<()> {
}

fn add_space_id_to_turbo_json(base: &CommandBase, space_id: &str) -> Result<()> {
let turbo_json_path = base.repo_root.join("turbo.json");
let turbo_json_path = base
.repo_root
.join_relative(RelativeSystemPathBuf::new("turbo.json").expect("relative"));

if !turbo_json_path.exists() {
return Err(anyhow!("turbo.json not found."));
Expand Down Expand Up @@ -453,6 +461,7 @@ mod test {

use tempfile::{NamedTempFile, TempDir};
use tokio::sync::OnceCell;
use turbopath::{AbsoluteSystemPathBuf, RelativeSystemPathBuf};
use vercel_api_mock::start_test_server;

use crate::{
Expand All @@ -468,6 +477,7 @@ mod test {
let user_config_file = NamedTempFile::new().unwrap();
fs::write(user_config_file.path(), r#"{ "token": "hello" }"#).unwrap();
let repo_config_file = NamedTempFile::new().unwrap();
let repo_config_path = AbsoluteSystemPathBuf::new(repo_config_file.path()).unwrap();
fs::write(
repo_config_file.path(),
r#"{ "apiurl": "http://localhost:3000" }"#,
Expand All @@ -487,7 +497,7 @@ mod test {
.unwrap(),
),
repo_config: OnceCell::from(
RepoConfigLoader::new(repo_config_file.path().to_path_buf())
RepoConfigLoader::new(repo_config_path)
.with_api(Some(format!("http://localhost:{}", port)))
.with_login(Some(format!("http://localhost:{}", port)))
.load()
Expand Down Expand Up @@ -517,6 +527,7 @@ mod test {

// repo config
let repo_config_file = NamedTempFile::new().unwrap();
let repo_config_path = AbsoluteSystemPathBuf::new(repo_config_file.path()).unwrap();
fs::write(
repo_config_file.path(),
r#"{ "apiurl": "http://localhost:3000" }"#,
Expand All @@ -526,7 +537,7 @@ mod test {
let port = port_scanner::request_open_port().unwrap();
let handle = tokio::spawn(start_test_server(port));
let mut base = CommandBase {
repo_root: TempDir::new().unwrap().into_path(),
repo_root: AbsoluteSystemPathBuf::new(TempDir::new().unwrap().into_path()).unwrap(),
ui: UI::new(false),
client_config: OnceCell::from(ClientConfigLoader::new().load().unwrap()),
user_config: OnceCell::from(
Expand All @@ -536,7 +547,7 @@ mod test {
.unwrap(),
),
repo_config: OnceCell::from(
RepoConfigLoader::new(repo_config_file.path().to_path_buf())
RepoConfigLoader::new(repo_config_path)
.with_api(Some(format!("http://localhost:{}", port)))
.with_login(Some(format!("http://localhost:{}", port)))
.load()
Expand All @@ -547,7 +558,10 @@ mod test {
};

// turbo config
let turbo_json_file = base.repo_root.join("turbo.json");
let turbo_json_file = base
.repo_root
.join_relative(RelativeSystemPathBuf::new("turbo.json").expect("relative"));

fs::write(
turbo_json_file.as_path(),
r#"{ "globalEnv": [], "pipeline": {} }"#,
Expand Down
7 changes: 5 additions & 2 deletions crates/turborepo-lib/src/commands/login.rs
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,7 @@ mod test {
use serde::Deserialize;
use tempfile::NamedTempFile;
use tokio::sync::OnceCell;
use turbopath::AbsoluteSystemPathBuf;
use vercel_api_mock::start_test_server;

use crate::{
Expand All @@ -325,6 +326,7 @@ mod test {
let user_config_file = NamedTempFile::new().unwrap();
fs::write(user_config_file.path(), r#"{ "token": "hello" }"#).unwrap();
let repo_config_file = NamedTempFile::new().unwrap();
let repo_config_path = AbsoluteSystemPathBuf::new(repo_config_file.path()).unwrap();
// Explicitly pass the wrong port to confirm that we're reading it from the
// manual override
fs::write(
Expand All @@ -343,7 +345,7 @@ mod test {
.unwrap(),
),
repo_config: OnceCell::from(
RepoConfigLoader::new(repo_config_file.path().to_path_buf())
RepoConfigLoader::new(repo_config_path)
.with_api(Some(format!("http://localhost:{}", port)))
.load()
.unwrap(),
Expand Down Expand Up @@ -376,6 +378,7 @@ mod test {
let user_config_file = NamedTempFile::new().unwrap();
fs::write(user_config_file.path(), r#"{ "token": "hello" }"#).unwrap();
let repo_config_file = NamedTempFile::new().unwrap();
let repo_config_path = AbsoluteSystemPathBuf::new(repo_config_file.path()).unwrap();
// Explicitly pass the wrong port to confirm that we're reading it from the
// manual override
fs::write(
Expand All @@ -394,7 +397,7 @@ mod test {
.unwrap(),
),
repo_config: OnceCell::from(
RepoConfigLoader::new(repo_config_file.path().to_path_buf())
RepoConfigLoader::new(repo_config_path)
.with_api(Some(format!("http://localhost:{}", port)))
.load()
.unwrap(),
Expand Down
32 changes: 25 additions & 7 deletions crates/turborepo-lib/src/commands/mod.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
use std::path::PathBuf;

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

use crate::{
Expand All @@ -22,7 +21,7 @@ pub(crate) mod logout;
pub(crate) mod unlink;

pub struct CommandBase {
pub repo_root: PathBuf,
pub repo_root: AbsoluteSystemPathBuf,
pub ui: UI,
user_config: OnceCell<UserConfig>,
repo_config: OnceCell<RepoConfig>,
Expand All @@ -32,7 +31,11 @@ pub struct CommandBase {
}

impl CommandBase {
pub fn new(args: Args, repo_root: PathBuf, version: &'static str) -> Result<Self> {
pub fn new(
args: Args,
repo_root: AbsoluteSystemPathBuf,
version: &'static str,
) -> Result<Self> {
Ok(Self {
repo_root,
ui: args.ui(),
Expand Down Expand Up @@ -162,19 +165,34 @@ impl CommandBase {
#[cfg(test)]
mod test {
use test_case::test_case;
use turbopath::AbsoluteSystemPathBuf;

use crate::get_version;

#[cfg(not(target_os = "windows"))]
#[test_case("/tmp/turborepo", "6e0cfa616f75a61c"; "basic example")]
Copy link
Member

Choose a reason for hiding this comment

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

Looks like windows isn't happy with /tmp/turborepo

#[test_case("", "e3b0c44298fc1c14"; "empty string ok")]
fn test_repo_hash(path: &str, expected_hash: &str) {
use std::path::PathBuf;
use super::CommandBase;
use crate::Args;

let args = Args::default();
let repo_root = AbsoluteSystemPathBuf::new(path).unwrap();
let command_base = CommandBase::new(args, repo_root, get_version()).unwrap();

let hash = command_base.repo_hash();

assert_eq!(hash, expected_hash);
assert_eq!(hash.len(), 16);
}

#[cfg(target_os = "windows")]
#[test_case("C:\\\\tmp\\turborepo", "0103736e6883e35f"; "basic example")]
fn test_repo_hash_win(path: &str, expected_hash: &str) {
use super::CommandBase;
use crate::Args;

let args = Args::default();
let repo_root = PathBuf::from(path);
let repo_root = AbsoluteSystemPathBuf::new(path).unwrap();
let command_base = CommandBase::new(args, repo_root, get_version()).unwrap();

let hash = command_base.repo_hash();
Expand Down
5 changes: 4 additions & 1 deletion crates/turborepo-lib/src/commands/unlink.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use std::fs::File;

use anyhow::{Context, Result};
use turbopath::RelativeSystemPathBuf;

use crate::{cli::LinkTarget, commands::CommandBase, config::TurboJson, ui::GREY};

Expand Down Expand Up @@ -53,7 +54,9 @@ pub fn unlink(base: &mut CommandBase, target: LinkTarget) -> Result<()> {
}

fn remove_spaces_from_turbo_json(base: &CommandBase) -> Result<UnlinkSpacesResult> {
let turbo_json_path = base.repo_root.join("turbo.json");
let turbo_json_path = base
.repo_root
.join_relative(RelativeSystemPathBuf::new("turbo.json").expect("relative"));

let turbo_json_file = File::open(&turbo_json_path).context("unable to open turbo.json file")?;
let mut turbo_json: TurboJson = serde_json::from_reader(turbo_json_file)?;
Expand Down
44 changes: 30 additions & 14 deletions crates/turborepo-lib/src/config/repo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use std::{
use anyhow::Result;
use config::Config;
use serde::{Deserialize, Serialize};
use turbopath::{AbsoluteSystemPathBuf, RelativeSystemPathBuf};

use super::{write_to_disk, MappedEnvironment};

Expand All @@ -17,7 +18,7 @@ const DEFAULT_LOGIN_URL: &str = "https://vercel.com";
pub struct RepoConfig {
disk_config: RepoConfigValue,
config: RepoConfigValue,
path: PathBuf,
path: AbsoluteSystemPathBuf,
}

#[derive(Debug, Deserialize, Serialize, Clone, PartialEq, Eq, Default)]
Expand All @@ -34,7 +35,7 @@ struct RepoConfigValue {

#[derive(Debug, Clone)]
pub struct RepoConfigLoader {
path: PathBuf,
path: AbsoluteSystemPathBuf,
api: Option<String>,
login: Option<String>,
team_slug: Option<String>,
Expand Down Expand Up @@ -77,18 +78,18 @@ impl RepoConfig {
}

fn write_to_disk(&self) -> Result<()> {
write_to_disk(&self.path, &self.disk_config)
write_to_disk(&self.path.as_path(), &self.disk_config)
}
}

#[allow(dead_code)]
pub fn get_repo_config_path(repo_root: &Path) -> PathBuf {
repo_root.join(".turbo").join("config.json")
pub fn get_repo_config_path(repo_root: &AbsoluteSystemPathBuf) -> AbsoluteSystemPathBuf {
let config = RelativeSystemPathBuf::new(".turbo/config.json").expect("is relative");
repo_root.join_relative(config)
}

impl RepoConfigLoader {
#[allow(dead_code)]
pub fn new(path: PathBuf) -> Self {
pub fn new(path: AbsoluteSystemPathBuf) -> Self {
Self {
path,
api: None,
Expand Down Expand Up @@ -188,7 +189,13 @@ mod test {

#[test]
fn test_repo_config_when_missing() -> Result<()> {
let config = RepoConfigLoader::new(PathBuf::from("missing")).load();
let path = if cfg!(windows) {
"C:\\missing"
} else {
"/missing"
};

let config = RepoConfigLoader::new(AbsoluteSystemPathBuf::new(path).unwrap()).load();
assert!(config.is_ok());

Ok(())
Expand All @@ -197,9 +204,10 @@ mod test {
#[test]
fn test_repo_config_with_team_and_api_flags() -> Result<()> {
let mut config_file = NamedTempFile::new()?;
let config_path = AbsoluteSystemPathBuf::new(config_file.path())?;
writeln!(&mut config_file, "{{\"teamId\": \"123\"}}")?;

let config = RepoConfigLoader::new(config_file.path().to_path_buf())
let config = RepoConfigLoader::new(config_path)
.with_team_slug(Some("my-team-slug".into()))
.with_api(Some("http://my-login-url".into()))
.load()?;
Expand All @@ -213,7 +221,13 @@ mod test {

#[test]
fn test_repo_config_includes_defaults() {
let config = RepoConfigLoader::new(PathBuf::from("missing"))
let path = if cfg!(windows) {
"C:\\missing"
} else {
"/missing"
};

let config = RepoConfigLoader::new(AbsoluteSystemPathBuf::new(path).unwrap())
.load()
.unwrap();
assert_eq!(config.api_url(), DEFAULT_API_URL);
Expand All @@ -225,9 +239,9 @@ mod test {
#[test]
fn test_team_override_clears_id() -> Result<()> {
let mut config_file = NamedTempFile::new()?;
let config_path = AbsoluteSystemPathBuf::new(config_file.path())?;
writeln!(&mut config_file, "{{\"teamId\": \"123\"}}")?;
let loader = RepoConfigLoader::new(config_file.path().to_path_buf())
.with_team_slug(Some("foo".into()));
let loader = RepoConfigLoader::new(config_path).with_team_slug(Some("foo".into()));

let config = loader.load()?;
assert_eq!(config.team_slug(), Some("foo"));
Expand All @@ -239,10 +253,11 @@ mod test {
#[test]
fn test_set_team_clears_id() -> Result<()> {
let mut config_file = NamedTempFile::new()?;
let config_path = AbsoluteSystemPathBuf::new(config_file.path())?;
// We will never pragmatically write the "teamslug" field as camelCase,
// but viper is case insensitive and we want to keep this functionality.
writeln!(&mut config_file, "{{\"teamSlug\": \"my-team\"}}")?;
let loader = RepoConfigLoader::new(config_file.path().to_path_buf());
let loader = RepoConfigLoader::new(config_path);

let mut config = loader.clone().load()?;
config.set_team_id(Some("my-team-id".into()))?;
Expand All @@ -257,12 +272,13 @@ mod test {
#[test]
fn test_repo_env_variable() -> Result<()> {
let mut config_file = NamedTempFile::new()?;
let config_path = AbsoluteSystemPathBuf::new(config_file.path())?;
writeln!(&mut config_file, "{{\"teamslug\": \"other-team\"}}")?;
let login_url = "http://my-login-url";
let api_url = "http://my-api";
let team_id = "123";
let team_slug = "my-team";
let config = RepoConfigLoader::new(config_file.path().to_path_buf())
let config = RepoConfigLoader::new(config_path)
.with_environment({
let mut env = HashMap::new();
env.insert("TURBO_API".into(), api_url.into());
Expand Down