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

Add Branch Selection for coffee_install() #242

Closed
wants to merge 4 commits into from
Closed
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
9 changes: 8 additions & 1 deletion coffee_cmd/src/cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ pub enum CoffeeCommand {
verbose: bool,
#[arg(short, long, action = clap::ArgAction::SetTrue)]
dynamic: bool,
#[arg(
name = "branch",
long,
help = "The branch to install the plugin from. If not specified, the default branch will be used."
)]
branch: Option<String>,
},
/// upgrade a single repository.
#[clap(arg_required_else_help = true)]
Expand Down Expand Up @@ -98,7 +104,8 @@ impl From<&CoffeeCommand> for coffee_core::CoffeeOperation {
plugin,
verbose,
dynamic,
} => Self::Install(plugin.to_owned(), *verbose, *dynamic),
branch,
} => Self::Install(plugin.to_owned(), *verbose, *dynamic, branch.clone()),
CoffeeCommand::Upgrade { repo, verbose } => Self::Upgrade(repo.to_owned(), *verbose),
CoffeeCommand::List {} => Self::List,
CoffeeCommand::Setup { cln_conf } => Self::Setup(cln_conf.to_owned()),
Expand Down
4 changes: 4 additions & 0 deletions coffee_cmd/src/coffee_term/command_show.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub fn show_list(coffee_list: Result<CoffeeList, CoffeeError>) -> Result<(), Cof
term::format::bold(String::from("Language")),
term::format::bold(String::from("Name")),
term::format::bold(String::from("Enabled")),
term::format::bold(String::from("Git HEAD")),
Copy link
Contributor

Choose a reason for hiding this comment

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

a user need also a git PHD to understand what is a git HEAD? Idk if this is necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already print the git HEAD to the user with coffee remote list
If we add the possibility to manage the branch for the plugin in separation from the repository, I think we have to print it in coffee list

Copy link
Contributor

Choose a reason for hiding this comment

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

Please call it commit there is no meaning to complicate a simple things with big words.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Do you want me to rename "Git HEAD" to "commit" in coffee remote list output too?

term::format::bold(String::from("Exec path")),
]);
table.divider();
Expand All @@ -30,6 +31,8 @@ pub fn show_list(coffee_list: Result<CoffeeList, CoffeeError>) -> Result<(), Cof
// Get whether the plugin is enabled
// If enabled is None, it means the plugin is enabled by default for backward compatibility.
let enabled = plugin.enabled.unwrap_or(true);
let mut commit = plugin.commit.clone().unwrap_or_default();
commit = commit.chars().take(7).collect::<String>();
table.push([
term::format::positive("●").into(),
term::format::highlight(plugin.lang.to_string()),
Expand All @@ -39,6 +42,7 @@ pub fn show_list(coffee_list: Result<CoffeeList, CoffeeError>) -> Result<(), Cof
} else {
term::format::negative("no").into()
},
term::format::primary(commit),
term::format::highlight(plugin.exec_path.to_owned()),
])
}
Expand Down
3 changes: 2 additions & 1 deletion coffee_cmd/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,14 @@ async fn run(args: CoffeeArgs, mut coffee: CoffeeManager) -> Result<(), CoffeeEr
plugin,
verbose,
dynamic,
branch,
} => {
let spinner = if !verbose {
Some(term::spinner("Compiling and installing"))
} else {
None
};
let result = coffee.install(&plugin, verbose, dynamic).await;
let result = coffee.install(&plugin, verbose, dynamic, branch).await;
if let Some(spinner) = spinner {
if result.is_ok() {
spinner.finish();
Expand Down
46 changes: 44 additions & 2 deletions coffee_core/src/coffee.rs
Original file line number Diff line number Diff line change
@@ -1,20 +1,21 @@
//! Coffee mod implementation
use std::collections::HashMap;
use std::fmt::Debug;
use std::vec::Vec;

Check warning on line 4 in coffee_core/src/coffee.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `Vec` is imported redundantly

Check warning on line 4 in coffee_core/src/coffee.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `Vec` is imported redundantly
use tokio::fs;

use async_trait::async_trait;
use clightningrpc_common::client::Client;
use clightningrpc_common::json_utils;
use clightningrpc_conf::{CLNConf, SyncCLNConf};
use log;

Check warning on line 11 in coffee_core/src/coffee.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `log` is imported redundantly

Check warning on line 11 in coffee_core/src/coffee.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `log` is imported redundantly
use serde::de::DeserializeOwned;
use serde::{Deserialize, Serialize};
use serde_json::json;
use tokio::process::Command;

use coffee_github::repository::Github;
use coffee_github::{git_upgrade_with_branch, git_upgrade_with_git_head};
use coffee_lib::errors::CoffeeError;
use coffee_lib::plugin_manager::PluginManager;
use coffee_lib::repository::Repository;
Expand Down Expand Up @@ -246,13 +247,37 @@
plugin: &str,
verbose: bool,
try_dynamic: bool,
branch: Option<String>,
) -> Result<(), CoffeeError> {
log::debug!("installing plugin: {plugin}");
// keep track if the plugin is successfully installed
for repo in self.repos.values() {
if let Some(mut plugin) = repo.get_plugin_by_name(plugin) {
log::trace!("{:?}", plugin);

// if the branch is specified, we need to save current git_head
// and checkout the branch
let current_git_head = repo.git_head();
let current_plugin_commit = plugin.commit.clone();
if let Some(branch) = branch.clone() {
log::debug!("checking out branch: {branch}");
if current_git_head.is_none() {
return Err(error!("unable to get the current git head"));
}
let updated_status =
git_upgrade_with_branch(repo.url().path_string.as_str(), &branch, verbose)
.await?;
log::debug!("updated status: {:?}", updated_status);

let repository = git2::Repository::open(repo.url().path_string.as_str())
.map_err(|err| error!("{}", err.message()))?;

// get commit id after the checkout
let commit = commit_id!(repository);
log::debug!("commit id after the checkout: {commit}");
plugin.commit = Some(commit);
}
Comment on lines +258 to +279
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO to much code for just sh!("git checkout {branch}")?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can see some potential issues with simply using git checkout {branch}:

  • If the branch is new (not fetched yet), we need to git fetch origin first.
    • To make sure we are returning to the expected state after switching back, we need the repository's Git HEAD, not just the current branch. This is because the branch might have been updated remotely (when we ran git fetch origin), and we want to go back to the exact commit the user was on.
    •  We need to verify that the repository's Git HEAD information exists (introduced in a previous PR as an Option to be backward compatible)
  • The plugin's commit field is updated during installation and then reverted afterward. Otherwise, we have false information.

Do you think that any of these steps is redundant?

Copy link
Contributor

Choose a reason for hiding this comment

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

my point is that we can do all with just a couple of git command, with your solution I do not see any fetch anywhere too and we need to make sure that your code is correct, otherwise we will have the same problem that we had with the upgrade logic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

git_upgrade_with_branch() does git fetch origin


// old_root_path is the path where the plugin is cloned and currently stored
// eg. ~/.coffee/repositories/<repo_name>/<plugin_name>
let old_root_path = plugin.root_path.clone();
Expand Down Expand Up @@ -291,7 +316,7 @@
if !try_dynamic {
// mark the plugin enabled
plugin.enabled = Some(true);
self.config.plugins.push(plugin);
self.config.plugins.push(plugin.clone());
log::debug!("path coffee conf: {}", self.coffee_cln_config.path);
self.coffee_cln_config
.add_conf("plugin", &path.to_owned())
Expand All @@ -302,6 +327,23 @@
} else {
self.start_plugin(&path).await?;
}

if branch.is_some() {
// we can safely unwrap here because we have checked it before
let current_git_head = current_git_head.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

the safety comment start with // SAFETY: ...

log::debug!("checking out to git head: {current_git_head}");
// rollback to the previous git head
let updated_status = git_upgrade_with_git_head(
repo.url().path_string.as_str(),
&current_git_head,
verbose,
)
.await?;
log::debug!("updated status: {:?}", updated_status);

plugin.commit = current_plugin_commit;
}
Comment on lines +331 to +345
Copy link
Contributor

Choose a reason for hiding this comment

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

wow this install method is starting to be 2 pages long?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree!
We can keep coffee install for just installing and configuring plugins and introduce coffee upgrade --plugin <plugin_name> --branch <branch_name> for upgrades and branch switching

Copy link
Contributor

Choose a reason for hiding this comment

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

No, my point is that the code that we are writing the code of the install is not good at all. Please see some of my PR that I am refactoring it.

In addition, both install and upgrade command will run the same logic and with your code we should duplicate the checkout logic. This is error prone

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, my point is that the code that we are writing the code of the install is not good at all.

Ok. how do you think I can make it better? I explained why we need the extra code in install() in this conversation #242 (comment).
Do you think that any of these steps is redundant or should be implemented in another way?

In addition, both install and upgrade command will run the same logic and with your code we should duplicate the checkout logic. This is error prone

They will have the same logic because they will do very similar tasks. The function git_upgrade_with_commit() will be reused in coffee upgrade --plugin <plugin_name>.
We can start with implementing coffee upgrade --plugin <plugin_name> and then use it directly from coffee install --branch <branch_name>. This way we don't duplicate code


return Ok(());
}
None => return Err(error!("exec path not found")),
Expand Down Expand Up @@ -360,7 +402,7 @@
UpgradeStatus::Updated(_, _) => {
for plugins in status.plugins_effected.iter() {
self.remove(plugins).await?;
self.install(plugins, verbose, false).await?;
self.install(plugins, verbose, false, None).await?;
}
}
_ => {}
Expand Down
2 changes: 1 addition & 1 deletion coffee_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ pub use coffee_lib as lib;
#[derive(Clone, Debug)]
pub enum CoffeeOperation {
/// Install(plugin name, verbose run, dynamic installation)
Install(String, bool, bool),
Install(String, bool, bool, Option<String>),
/// List
List,
// Upgrade(name of the repository, verbose run)
Expand Down
2 changes: 2 additions & 0 deletions coffee_github/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
pub mod repository;
mod utils;

pub use utils::{git_upgrade_with_branch, git_upgrade_with_git_head};

#[cfg(test)]
mod tests {
use std::{path::Path, sync::Once};
Expand Down
9 changes: 7 additions & 2 deletions coffee_github/src/repository.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::any::Any;

use async_trait::async_trait;
use git2;

Check warning on line 4 in coffee_github/src/repository.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `git2` is imported redundantly

Check warning on line 4 in coffee_github/src/repository.rs

View workflow job for this annotation

GitHub Actions / Build (nightly)

the item `git2` is imported redundantly
use log::debug;
use tokio::fs::File;
use tokio::io::AsyncReadExt;
Expand All @@ -21,7 +21,7 @@
use coffee_storage::model::repository::Repository as StorageRepository;

use crate::utils::clone_recursive_fix;
use crate::utils::git_upgrade;
use crate::utils::git_upgrade_with_branch;

pub struct Github {
/// the url of the repository to be able
Expand Down Expand Up @@ -259,7 +259,7 @@
}
}
// pull the changes from the repository
let status = git_upgrade(&self.url.path_string, &self.branch, verbose).await?;
let status = git_upgrade_with_branch(&self.url.path_string, &self.branch, verbose).await?;
self.git_head = Some(status.commit_id());
self.last_activity = Some(status.date());
Ok(CoffeeUpgrade {
Expand Down Expand Up @@ -336,6 +336,11 @@
None
}

/// return the git head of the repository.
fn git_head(&self) -> Option<String> {
self.git_head.clone()
}
Comment on lines +339 to +342
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this as a struct method and not an a trait implementation


fn as_any(&self) -> &dyn Any {
self
}
Expand Down
26 changes: 25 additions & 1 deletion coffee_github/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub async fn clone_recursive_fix(repo: git2::Repository, url: &URL) -> Result<()
Ok(())
}

pub async fn git_upgrade(
pub async fn git_upgrade_with_branch(
Copy link
Contributor

Choose a reason for hiding this comment

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

why changing the name here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

because I added another function that uses the git commit to upgrade

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the point of changing the name, due that there is no git upgrade command in git

Copy link
Contributor

Choose a reason for hiding this comment

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

Moreover, git uses always a branch there is never a without_branch cases

path: &str,
branch: &str,
verbose: bool,
Expand All @@ -48,3 +48,27 @@ pub async fn git_upgrade(
Ok(UpgradeStatus::Updated(upstream_commit, date))
}
}

pub async fn git_upgrade_with_git_head(
path: &str,
git_head: &str,
verbose: bool,
) -> Result<UpgradeStatus, CoffeeError> {
use tokio::process::Command;
Comment on lines +52 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

idk if this is a good idea, the checkout is just git checkout <branch-name>

Why we need to use all this code?


let repo = git2::Repository::open(path).map_err(|err| error!("{}", err.message()))?;

let (local_commit, _) = get_repo_info!(repo);

let mut cmd = format!("git fetch origin\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

let mut cmd = "git fetch origin\n";

format! is not needed

cmd += &format!("git reset --hard {}", git_head);
sh!(path, cmd, verbose);

let (upstream_commit, date) = get_repo_info!(repo);

if local_commit == upstream_commit {
Ok(UpgradeStatus::UpToDate(upstream_commit, date))
} else {
Ok(UpgradeStatus::Updated(upstream_commit, date))
}
}
2 changes: 1 addition & 1 deletion coffee_httpd/src/httpd/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ async fn coffee_install(
let try_dynamic = body.try_dynamic;

let mut coffee = data.coffee.lock().await;
let result = coffee.install(plugin, false, try_dynamic).await;
let result = coffee.install(plugin, false, try_dynamic, None).await;

handle_httpd_response!(result, "Plugin '{plugin}' installed successfully")
}
Expand Down
1 change: 1 addition & 0 deletions coffee_lib/src/plugin_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub trait PluginManager {
plugins: &str,
verbose: bool,
try_dynamic: bool,
branch: Option<String>,
) -> Result<(), CoffeeError>;

// remove a plugin by name, return an error if some error happens.
Expand Down
3 changes: 3 additions & 0 deletions coffee_lib/src/repository.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,5 +40,8 @@ pub trait Repository: Any {
/// return the url of the repository.
fn url(&self) -> URL;

/// return the git head of the repository.
fn git_head(&self) -> Option<String>;

Comment on lines +43 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

does not make sense to have this inside the repository trait

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. I will update it

fn as_any(&self) -> &dyn Any;
}
2 changes: 1 addition & 1 deletion coffee_plugin/src/plugin/plugin_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ fn coffee_install(plugin: &mut Plugin<State>, request: Value) -> Result<Value, P
let rt = Runtime::new().unwrap();

let request: InstallReq = serde_json::from_value(request)?;
rt.block_on(coffee.install(&request.name, false, true))
rt.block_on(coffee.install(&request.name, false, true, None))
.map_err(from)?;
Ok(json!({}))
}
Expand Down
8 changes: 8 additions & 0 deletions docs/docs-book/src/using-coffee.md
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,14 @@ To install a plugin statically, you simply need to run.
coffee install <plugin_name>
```

#### Specifying a branch for installation

To install a plugin from a specific branch, you simply need to run.

```bash
coffee install <plugin_name> --branch <branch_name>
```

Comment on lines +107 to +114
Copy link
Contributor

Choose a reason for hiding this comment

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

we should not document every single command possibility otherwise our documentation will be always uncompleted

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. I can remove this section

Copy link
Contributor

Choose a reason for hiding this comment

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

but we should came up with a way to document optional parameters

### Removing a Plugin

> ✅ Implemented
Expand Down
20 changes: 10 additions & 10 deletions tests/src/coffee_integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ pub async fn init_coffee_test_add_remote() {
.unwrap();
manager
.coffee()
.install("summary", true, true)
.install("summary", true, true, None)
.await
.unwrap();

Expand Down Expand Up @@ -167,13 +167,13 @@ pub async fn test_add_remove_plugins() {
);

// Install summary plugin
let result = manager.coffee().install("summary", true, false).await;
let result = manager.coffee().install("summary", true, false, None).await;
assert!(result.is_ok(), "{:?}", result);

// Install helpme plugin
manager
.coffee()
.install("helpme", true, false)
.install("helpme", true, false, None)
.await
.unwrap();

Expand Down Expand Up @@ -276,7 +276,7 @@ pub async fn test_errors_and_show() {
);

// Install summary plugin
let result = manager.coffee().install("summary", true, false).await;
let result = manager.coffee().install("summary", true, false, None).await;
assert!(result.is_ok(), "{:?}", result);

// Get the README file for a plugin that is not installed
Expand All @@ -285,7 +285,7 @@ pub async fn test_errors_and_show() {
assert!(val.starts_with("# Helpme plugin"));

// Install a plugin that is not in the repository
let result = manager.coffee().install("x", true, false).await;
let result = manager.coffee().install("x", true, false, None).await;
assert!(result.is_err(), "{:?}", result);

// Remove helpme plugin
Expand Down Expand Up @@ -353,7 +353,7 @@ pub async fn install_plugin_in_two_networks() -> anyhow::Result<()> {
// This should install summary plugin for regtest network
manager
.coffee()
.install("summary", true, true)
.install("summary", true, true, None)
.await
.unwrap();
// Ensure that summary is installed for regtest network
Expand Down Expand Up @@ -393,7 +393,7 @@ pub async fn install_plugin_in_two_networks() -> anyhow::Result<()> {
// This should install summary plugin for testnet network
manager
.coffee()
.install("summary", true, true)
.install("summary", true, true, None)
.await
.unwrap();
// Ensure that summary is installed for testnet network
Expand Down Expand Up @@ -428,13 +428,13 @@ pub async fn test_double_slash() {
.unwrap();

// Install summary plugin
let result = manager.coffee().install("summary", true, false).await;
let result = manager.coffee().install("summary", true, false, None).await;
assert!(result.is_ok(), "{:?}", result);

// Install helpme plugin
manager
.coffee()
.install("helpme", true, false)
.install("helpme", true, false, None)
.await
.unwrap();

Expand Down Expand Up @@ -493,7 +493,7 @@ pub async fn test_plugin_installation_path() {
// Install summary plugin for regtest network
manager
.coffee()
.install("summary", true, false)
.install("summary", true, false, None)
.await
.unwrap();

Expand Down
Loading