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

git: adding basic function to checkout a branch #248

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

vincenzopalazzo
Copy link
Contributor

@vincenzopalazzo vincenzopalazzo commented Feb 26, 2024

Alternative version of #242

Fixes #205

Copy link

netlify bot commented Feb 26, 2024

Deploy Preview for coffee-docs canceled.

Name Link
🔨 Latest commit 4d7920d
🔍 Latest deploy log https://app.netlify.com/sites/coffee-docs/deploys/661c596aa2c1f40008685bcb

@@ -1,8 +1,10 @@
use log::debug;
use tokio::process::Command;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrong import place

Copy link
Collaborator

@tareknaser tareknaser left a comment

Choose a reason for hiding this comment

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

Thanks

Here are some notes as well:

  • We need to update integration tests in tests/src/coffee_integration_tests.rs to pass branch parameter to coffee_install
  • coffee plugin needs to pass the branch value as well. See coffee_plugin/src/plugin/plugin_mod.rs
  • We can also add a comment in the docs to mention the new parameter

if let Some(branch) = branch {
// FIXME: Where we store the date? how we manage it?
let (commit, _) =
git_checkout(&plugin.root_path, &branch, verbose).await?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We switch the whole repository to the required branch but we don't switch back to the original branch of the repository after installing the plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice point

@vincenzopalazzo
Copy link
Contributor Author

Ok, this needs to go in before the first release, to fix the CI.

We can add the remote with a specific commit when summary was sane

@vincenzopalazzo vincenzopalazzo added the P-hight Hight Priotity issue label Apr 14, 2024
@vincenzopalazzo vincenzopalazzo self-assigned this Apr 14, 2024
@tareknaser
Copy link
Collaborator

We can add the remote with a specific commit when summary was sane

Is this possible with git?
if not, we can force push it manually in the tests

What do you think?

@vincenzopalazzo
Copy link
Contributor Author

Is this possible with git?
if not, we can force push it manually in the tests

Yeah make sense to do it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-hight Hight Priotity issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Branch Specification for Plugin Installation
2 participants