-
Notifications
You must be signed in to change notification settings - Fork 257
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
feat: use cargo generate
instead of hardcoding examples source code
#888
Conversation
@oddgrd what do you think of this approach using It requires the user to have In theory it's possible to include |
Oh, cool, I didn't know about this crate! At first glance using that as a library seems like a great idea. That's a shame about the dependency conflicts though, I have ran into issues with the git2 dependency in the past (as you can see from the comment above it). We would prefer to avoid needing cargo-generate to be installed, since that can lead to poor DX. We can install it for them, but that can also be finicky to get working smoothly across systems. 🤔 We don't want to reinvent the wheel either though... I think we should explore further if we can use it as a library (aka if we can resolve the dependency conflicts). |
The dependency conflict should also be easier to resolve after we remove the |
I think I'm going to put this issue on ice until we've removed the |
@paulotten just out of curiosity: the |
That needs to go too. |
Oh, that makes sense. Ok, thanks for the clarification! 🙏 |
#756 was resolved by #922 . @paulotten I am curious if we can continue with this PR. Do you have the time to take another stab at it? The idea is that we want to close #821 pretty soon and we're interested in getting this PR over the line. Let us know what you think |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason it doesn't work well on my side. Doing an init for an axum
project brings over the entire shuttle-examples repo. Can you reproduce it?
I also get the entire repo |
You can bump the examples submodule to the latest on main :) |
Should be fixed now. Now I'm noticing that the crate name isn't getting set correctly. Looking into it. |
Fixed, I think. |
Should be fixed. (Ended up using |
Looks like just a new line at end of file difference. I'll leave it to you. |
Yeah, but there are also some tests that fail sporadically. I think I've managed to reproduce it. This can be merged if the team wants, and I'll add the fixes to the subsequent PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always love to see a PR that removes code.
Small nit: maybe the Shuttle.toml
should be deleted since it will override the project to something that already exists.
@chesedo Yeah, either that or make it always create a Shuttle.toml. If many more features are planned for that file, then we might want it to be created in every project. (region, environments ... ???) |
Restating the problem to make sure I understand it: Many (but not all) of the examples have a So if we find a Shuttle.toml file after copying the example, we should delete it. Ya, I can do that. |
There should be an update to this PR for this commit 65ab862 GitHub is having issues though. https://www.githubstatus.com/ |
Appears to be fixed. I see the expected commit now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks a lot for this. 🙏
I saw now that two things are missing! The |
Good catch! Should we include that here, or have you started on these in your new PR @jonaro00? |
I have not started on those. You can do them here if you want. |
|
Thanks, sounds good! |
Great. |
The simple version is if the template already has a .gitignore file, just use that that instead. |
Good point! Another thought, can we use the git2 library for this? |
It depends how complex we need to make this. If we need something like this, I'd probably go reaching for a |
So shall we add a .gitignore with |
We can still pass that option so that a git repo is init'ed. |
For git2 I was just thinking it would be nice if we could use that since we already depend on it, but if that gets complicated it's not worth it for this PR. I think we can do your suggestion Paul of generating the .gitignore if it doesn't exist, but I also think we can leave it out of this PR if we add a .gitignore to all examples as @jonaro00 suggested (which we should do either way). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for finding the cargo-generate
solution and for finishing this up so quickly after the cargo removal! ❤️
Description of change
Use
cargo-generate
(as a library) to bootstrap copies ofshuttle-examples
apps.Closes #867.
How Has This Been Tested (if applicable)?
Ran locally.