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

feat: use cargo generate instead of hardcoding examples source code #888

Merged
merged 14 commits into from
Jun 9, 2023

Conversation

paulotten
Copy link
Contributor

@paulotten paulotten commented May 8, 2023

Description of change

Use cargo-generate (as a library) to bootstrap copies of shuttle-examples apps.

Closes #867.

How Has This Been Tested (if applicable)?

Ran locally.

cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved
@paulotten
Copy link
Contributor Author

paulotten commented May 8, 2023

@oddgrd what do you think of this approach using cargo generate?

It requires the user to have cargo generate installed, which I'll have to add documentation for.

In theory it's possible to include cargo-generate as a library instead. In practice I haven't been able to make the dependencies compile. (cargo-generate=0.18 requires git2=0.17, which conflicts with other things we use. cargo-generate=0.17 requires git2=0.16 which we use, but then fails to compile. cargo-generate=0.16 requires git2=0.14 which we is too low.)

@oddgrd
Copy link
Contributor

oddgrd commented May 9, 2023

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).

@oddgrd
Copy link
Contributor

oddgrd commented May 9, 2023

The dependency conflict should also be easier to resolve after we remove the cargo dependency, which is in progress. The cargo dependency currently ties down the git2 version.

@paulotten
Copy link
Contributor Author

The dependency conflict should also be easier to resolve after we remove the cargo dependency, which is in progress. The cargo dependency currently ties down the git2 version.

I think I'm going to put this issue on ice until we've removed the cargo dependency.

@iulianbarbu
Copy link
Contributor

iulianbarbu commented May 22, 2023

@paulotten just out of curiosity: the cargo-shuttle dependency on cargo was removed here: https://github.com/shuttle-hq/shuttle/pull/765/files. Would you still be blocked by the cargo dependency that we have in the other places, when trying to fix the conflicts coming from git2 dependency?

@paulotten
Copy link
Contributor Author

@paulotten just out of curiosity: the cargo-shuttle dependency on cargo was removed here: https://github.com/shuttle-hq/shuttle/pull/765/files. Would you still be blocked by the cargo dependency that we have in the other places, when trying to fix the conflicts coming from git2 dependency?

cargo is still a requirement in the top level (workspace) Cargo.toml. https://github.com/shuttle-hq/shuttle/blob/main/Cargo.toml#L44

That needs to go too.

@iulianbarbu
Copy link
Contributor

Oh, that makes sense. Ok, thanks for the clarification! 🙏

@iulianbarbu
Copy link
Contributor

#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

@paulotten paulotten marked this pull request as ready for review June 6, 2023 17:50
Copy link
Contributor

@iulianbarbu iulianbarbu left a 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?

cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/init.rs Outdated Show resolved Hide resolved
@jonaro00
Copy link
Member

jonaro00 commented Jun 7, 2023

I also get the entire repo

@jonaro00
Copy link
Member

jonaro00 commented Jun 7, 2023

You can bump the examples submodule to the latest on main :)

@paulotten
Copy link
Contributor Author

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?

Should be fixed now.

Now I'm noticing that the crate name isn't getting set correctly. Looking into it.

@paulotten
Copy link
Contributor Author

You can bump the examples submodule to the latest on main :)

Fixed, I think.

@paulotten
Copy link
Contributor Author

Now I'm noticing that the crate name isn't getting set correctly. Looking into it.

Should be fixed. (Ended up using toml-edit to set it.)

@paulotten
Copy link
Contributor Author

Maybe just a tiny nitpick: the toml_edit imports could be at top of file.

Document and value feel pretty generic to be using everywhere in the file. That said we did have that before I started modifying this file. I'll move it back to the top.

I'm looking at if we can fix the failing rocket init tests and kill two birds with one stone.

Looks like just a new line at end of file difference. I'll leave it to you.

@jonaro00
Copy link
Member

jonaro00 commented Jun 7, 2023

Looks like just a new line at end of file difference.

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.

Copy link
Contributor

@chesedo chesedo left a 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.

cargo-shuttle/Cargo.toml Show resolved Hide resolved
@jonaro00
Copy link
Member

jonaro00 commented Jun 7, 2023

@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 ... ???)

@paulotten
Copy link
Contributor Author

Small nit: maybe the Shuttle.toml should be deleted since it will override the project to something that already exists.

Restating the problem to make sure I understand it:

Many (but not all) of the examples have a Shuttle.toml file. This file overrides the Shuttle project name to a value that is not useful.

So if we find a Shuttle.toml file after copying the example, we should delete it.


Ya, I can do that.

@paulotten paulotten requested a review from chesedo June 7, 2023 18:27
@paulotten
Copy link
Contributor Author

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/

Incident with Actions, Pull Requests and Webhooks

@paulotten
Copy link
Contributor Author

GitHub is having issues though.

Appears to be fixed. I see the expected commit now.

Copy link
Contributor

@iulianbarbu iulianbarbu left a 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. 🙏

@jonaro00
Copy link
Member

jonaro00 commented Jun 7, 2023

I saw now that two things are missing! The init flag to generate does not initialize a git repo like cargo init does. One more thing that cargo does is add a .gitignore with target/. We should probably add this aswell but we can also include Secrets*.toml!

@oddgrd
Copy link
Contributor

oddgrd commented Jun 8, 2023

I saw now that two things are missing! The init flag to generate does not initialize a git repo like cargo init does. One more thing that cargo does is add a .gitignore with target/. We should probably add this aswell but we can also include Secrets*.toml!

Good catch! Should we include that here, or have you started on these in your new PR @jonaro00?

@jonaro00
Copy link
Member

jonaro00 commented Jun 8, 2023

I have not started on those. You can do them here if you want.

@paulotten
Copy link
Contributor Author

One more thing that cargo does is add a .gitignore with target/.

cargo_generate has a vcs option. Setting it to Git however doesn't seem to create a .gitignore.

We should probably add this aswell but we can also include Secrets*.toml!

cargo_generate isn't going to know to add that. I'll just write a .gitignore file manually.

@oddgrd
Copy link
Contributor

oddgrd commented Jun 8, 2023

One more thing that cargo does is add a .gitignore with target/.

cargo_generate has a vcs option. Setting it to Git however doesn't seem to create a .gitignore.

We should probably add this aswell but we can also include Secrets*.toml!

cargo_generate isn't going to know to add that. I'll just write a .gitignore file manually.

Thanks, sounds good!

@jonaro00
Copy link
Member

jonaro00 commented Jun 8, 2023

Great.
The template could have a gitignore already. Should we append to the file if it exists? Or parse it and only add the lines if they are missing?

@paulotten
Copy link
Contributor Author

The template could have a gitignore already. Should we append to the file if it exists? Or parse it and only add the lines if they are missing?

The simple version is if the template already has a .gitignore file, just use that that instead.

@oddgrd
Copy link
Contributor

oddgrd commented Jun 8, 2023

Good point! Another thought, can we use the git2 library for this?

@paulotten
Copy link
Contributor Author

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 gix crate (Rust rewrite of git functionality). Maybe gix_ignore.

@jonaro00
Copy link
Member

jonaro00 commented Jun 8, 2023

So shall we add a .gitignore with target/ and Secrets*.toml to all examples? It seems reasonable that they have this as a standard now that cloning will be the default way of starting off.

@jonaro00
Copy link
Member

jonaro00 commented Jun 8, 2023

Setting it to Git however doesn't seem to create a .gitignore.

We can still pass that option so that a git repo is init'ed.

@oddgrd
Copy link
Contributor

oddgrd commented Jun 8, 2023

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).

Copy link
Contributor

@oddgrd oddgrd left a 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! ❤️

@oddgrd oddgrd merged commit 8bb05b6 into shuttle-hq:main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Refactor init to bootstrap examples
5 participants