feat: init a Git repo for default use_case template #97

Angel-Dijoux · 2024-11-05T15:53:12Z

No description provided.

volgar1x · 2024-11-12T15:49:34Z

Cargo.toml

@@ -92,6 +93,9 @@ tracing-subscriber = { version = "0.3", features = ["json", "env-filter"] }
 url = { version = "2.5", features = ["serde"] }
 uuid = "1.7"
 which = "6.0"
+openssl = { version = "0.10.68", features = ["vendored"], optional = true }
+openssl-probe = { version = "0.1.5", optional = true }
+git2 = { version = "0.19.0" , features = ["vendored-openssl"] }


What's the reason for using the git2 crate instead of the user's installed git binary?

I don't want to rely on the user's environment to be honest, also on our side it is easier to manipulate the library compared to system commands

Relying on the user's environment in this case might be beneficial because what's the point of initializing a git repo if they do not have git installed?

easier to manipulate would also be an overstretch in my humble opinion.

I just wanted to advocate for this solution because it might simplify our CI too. Everything looks good apart from that 😉

I also thought about relying on the user's git, but I looked at how cargo handles it, and they do use git2 so I think this solution is fine. In any case it's not a huge deal for someone to have an unnecessary .git directory

jpopesculian

Had a quick peek, and I would say we should stick to only creating the empty git directly and not be doing any commits, I wouldn't want to mess with anyone's commit history and we also can't be certain which files to add and that can be really frustrating to reverse

jpopesculian · 2024-11-20T14:49:06Z

.github/workflows/cd.yaml

+              "aarch64" | "armv7" | "s390x" | "ppc64le")
+                  # NOTE: pypa/manylinux docker images are Debian based
+                  sudo apt-get update
+                  sudo apt-get install -y pkg-config libssl-dev


can we just install libgit2-dev directly instead of these custom packages? Might make it easier to maintain and more clear what's going on

It's possible, I did it, and I also moved it into a script to make it clearer!

Angel-Dijoux · 2024-11-20T18:07:22Z

Had a quick peek, and I would say we should stick to only creating the empty git directly and not be doing any commits, I wouldn't want to mess with anyone's commit history and we also can't be certain which files to add and that can be really frustrating to reverse

Oh yes! You're right. I thought it was a good idea, but from this point of view, not really I agree, it's been removed thanks

jpopesculian

Looking good! Let me know if it works to get rid of the default features for git2 and avoid the hassle with openssl

jpopesculian · 2024-11-22T15:12:36Z

Cargo.toml

@@ -19,6 +19,7 @@ name = "aqora"
 [features]
 default = []
 extension-module = ["pyo3/extension-module", "pyo3/abi3-py39", "pyo3/abi3"]
+vendored-openssl = ["openssl/vendored"]


Suggested change

vendored-openssl = ["openssl/vendored"]

jpopesculian · 2024-11-22T15:13:44Z

Cargo.toml

+openssl = { version = "0.10.68", features = ["vendored"], optional = true }
+openssl-probe = { version = "0.1.5", optional = true }
+git2 = { version = "0.19.0" , features = ["vendored-openssl"] }


Suggested change

openssl = { version = "0.10.68", features = ["vendored"], optional = true }

openssl-probe = { version = "0.1.5", optional = true }

git2 = { version = "0.19.0" , features = ["vendored-openssl"] }

git2 = { version = "0.19.0", default-features = false }

We don't need to use ssh or https because we don't ever interact with remote servers

Well, the CI/CD is working fine now with this. I don't remember why I got into this (maybe the commit?)

jpopesculian · 2024-11-22T15:19:33Z

.github/workflows/cd.yaml

+      - name: Calculate openssl-vendored
+        shell: bash
+        id: is-openssl-vendored
+        run: |


I would just get rid of all of the openssl stuff

jpopesculian · 2024-11-22T15:21:52Z

src/commands/template.rs

-            destination.display()
-        )),
+        Ok(_) => {
+            init_repository(&pb, global.project.as_path(), None).map_err(|err| {


why not stick format_permission_error in crate::error and reuse it here? also init_repository could also just return create::Result<()> to save yourself some unnecessary error formatting

jpopesculian · 2024-11-22T15:23:09Z

src/git.rs

+            .as_str(),
+    )
+    .no_reinit(true)
+    .initial_head("main");


you'd have to check if this gets automatically putlled from the users .gitconfig. If it does, I wouldn't want to mess with their local configuration

Ahh yep it's overriding user .gitconfig conf /: I've removed it!

jpopesculian

Sorry! Last things

jpopesculian · 2024-11-30T12:23:48Z

src/commands/new/use_case.rs

-            )
-        })?;
+        .map_err(|e| format_permission_error("create use case", &dest, &e))?;
+    init_repository(&pb, &dest, Some(competition.short_description))?;


this should probably just be a warning on fail

jpopesculian · 2024-11-30T12:27:51Z

src/commands/template.rs

-            destination.display()
-        )),
+        Ok(_) => {
+            init_repository(&pb, &destination, None)?;


same here: just a warning

jpopesculian · 2024-11-30T12:29:56Z

Cargo.toml

+openssl = { version = "0.10.68", features = ["vendored"], optional = true }
+openssl-probe = { version = "0.1.5", optional = true }


Suggested change

openssl = { version = "0.10.68", features = ["vendored"], optional = true }

openssl-probe = { version = "0.1.5", optional = true }

are these still necessary?

Oh, you're right, they're not. Thanks!

Angel-Dijoux force-pushed the angel/init_git_use_case branch from f7a9870 to 5e60549 Compare November 8, 2024 10:00

Angel-Dijoux requested a review from jpopesculian November 9, 2024 10:42

volgar1x reviewed Nov 12, 2024

View reviewed changes

Angel-Dijoux force-pushed the angel/init_git_use_case branch 3 times, most recently from 8a66a45 to 1acc3c9 Compare November 15, 2024 00:56

Angel-Dijoux self-assigned this Nov 15, 2024

jpopesculian requested changes Nov 20, 2024

View reviewed changes

Angel-Dijoux force-pushed the angel/init_git_use_case branch from 7f71138 to ea47281 Compare November 20, 2024 18:00

Angel-Dijoux requested a review from jpopesculian November 20, 2024 18:20

jpopesculian requested changes Nov 22, 2024

View reviewed changes

Angel-Dijoux force-pushed the angel/init_git_use_case branch 2 times, most recently from d073bf2 to 4863d30 Compare November 25, 2024 20:16

Angel-Dijoux requested a review from jpopesculian November 25, 2024 20:19

jpopesculian requested changes Nov 30, 2024

View reviewed changes

Angel-Dijoux requested a review from jpopesculian November 30, 2024 13:00

Angel-Dijoux force-pushed the angel/init_git_use_case branch from d951de2 to 6b1aaf1 Compare December 5, 2024 19:05

Angel-Dijoux added 8 commits December 7, 2024 11:58

feat: init a repo for default use_case template

d3d06f2

fix: pr suggestions

de89287

feat: create repo after cloning template & commit after tests

d00d2d2

chore: remove x86 support (rust-lang/cargo#13546)

edda06d

ci: uselibgit2-dev only

10ac91e

fix: remove commit

6949ac2

chore: cleanup & test ci without ssl feature

e1976b4

fix: remove openssl and warm user if we can't set a git repo

6bee773

jpopesculian force-pushed the angel/init_git_use_case branch from 6b1aaf1 to 6bee773 Compare December 7, 2024 10:58

jpopesculian approved these changes Dec 7, 2024

View reviewed changes

jpopesculian merged commit 0d7408d into main Dec 7, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: init a Git repo for default use_case template #97

feat: init a Git repo for default use_case template #97

Angel-Dijoux commented Nov 5, 2024

volgar1x Nov 12, 2024

Angel-Dijoux Nov 12, 2024

volgar1x Nov 13, 2024

jpopesculian Nov 20, 2024

jpopesculian left a comment

jpopesculian Nov 20, 2024

Angel-Dijoux Nov 20, 2024

Angel-Dijoux commented Nov 20, 2024

jpopesculian left a comment

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

Angel-Dijoux Nov 25, 2024

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

jpopesculian Nov 22, 2024

Angel-Dijoux Nov 25, 2024

jpopesculian left a comment

jpopesculian Nov 30, 2024

jpopesculian Nov 30, 2024

jpopesculian Nov 30, 2024

Angel-Dijoux Nov 30, 2024

		openssl = { version = "0.10.68", features = ["vendored"], optional = true }
		openssl-probe = { version = "0.1.5", optional = true }

feat: init a Git repo for default use_case template #97

feat: init a Git repo for default use_case template #97

Conversation

Angel-Dijoux commented Nov 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Angel-Dijoux commented Nov 20, 2024

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpopesculian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment