Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: init new git repo for new projects #558

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jacobhq
Copy link
Contributor

@jacobhq jacobhq commented Feb 12, 2025

Checklist

Related issue

Fixes #554

Overview

Checks if git is installed, and init a new repo by default. If git is not installed, we continue in silence, and only fail with error if the user specifies the git flag, but git is not installed.

Tasks

  • Ensure git is available in CI to run tests
  • Write unit test for function
  • Write integration test for all cases
  • Add flag to docs (not much to do, but will update the help snippet on the cli page)

@github-actions github-actions bot added the rust Requires rust knowledge label Feb 12, 2025
@jacobhq

This comment was marked as resolved.

@marcalexiei

This comment was marked as outdated.

@marcalexiei

This comment was marked as resolved.

@jacobhq

This comment was marked as resolved.

@jacobhq jacobhq marked this pull request as ready for review February 12, 2025 19:43
Copy link
Member

@marcalexiei marcalexiei left a comment

Choose a reason for hiding this comment

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

I added a few comments to refine messages and comments.
Let me know what do you think 😉

(The code review will still be performed by @Valerioageno since I'm not a rust expert 😅)

crates/tuono/src/cli.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
@jacobhq
Copy link
Contributor Author

jacobhq commented Feb 12, 2025

I added a few comments to refine messages and comments.

Thank you! What do you think of my proposed logic for the git workflow (description in PR overview)?

crates/tuono/src/cli.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Anyway cargo test is failing again because the command output is:

Failed to parse the tag response

rather than exiting with a success status code.
This makes me wondering if there is a temporary issue with GitHub API since rerunning produced, again, no errors.

@Valerioageno I think this should be resolved by using a mock server when executing unit tests,
which is already worked on #411.
However the work is halted due to a missing release of .dotenvy crate.
Maybe there is another way to go ahead?
E.g. Creating a trait hardcoding the GitHub domain for production and mocking it with the local mock server during unit tests?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely needs to create the mock first.

Let's put these tests in a separate branch and merging them once the above PR gets merged.

Note: These tests should be put in tests/cli_new.rs

crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
crates/tuono/src/scaffold_project.rs Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, definitely needs to create the mock first.

Let's put these tests in a separate branch and merging them once the above PR gets merged.

Note: These tests should be put in tests/cli_new.rs

#[test]
#[serial]
fn it_inits_new_git_repo_by_default_with_git_installed() {
let temp_tuono_project = TempTuonoProject::new();
Copy link
Member

Choose a reason for hiding this comment

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

The same problem in the unit test happens here (if the host machine has not installed git the tests fail).
We should consider mocking it, otherwise we gonna increase test complexity and flakiness

@jacobhq
Copy link
Contributor Author

jacobhq commented Feb 13, 2025

Please can I have a bit of help here? When I use TempTuonoProject in the cli_build.rs file, there are no errors. However, because the rust compiler treats these tests separately, when I use the module in the cli_new.rs file, the compiler errors because in this test, the function is not used.

I could ignore the dead code warning, but that feels bad. WDYT?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
rust Requires rust knowledge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic git init
3 participants