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: shuttle init --from #984

Merged
merged 19 commits into from
Jun 20, 2023
Merged

Conversation

jonaro00
Copy link
Member

@jonaro00 jonaro00 commented Jun 8, 2023

Description of change

Closes #867 (part 2), Follow-up on #888

Allows you to init by cloning any repo with

  • cargo shuttle init --from https://github.com/abc/123 --subfolder path/to/template
  • cargo shuttle init --from gh:abc/123 --subfolder path/to/template # gh:, gl:, bb:
  • cargo shuttle init --from abc/123 --subfolder path/to/template
  • cargo shuttle init --from ../some/local/repo --subfolder path/to/template

User is prompted if they want to use a different example template than hello-world: (also added link to examples repo; the README there should change to suggest using the above commands)

image

Improved post-init:

You can `cd` to the directory, then:
Run `cargo shuttle run` to run the app locally.
Run `cargo shuttle project start` to create a project environment on Shuttle.

Also bumped Cargo.lock

How has this been tested? (if applicable)

Testing locally for various combinations of flags and expanded test cases to cover everything new

@jonaro00
Copy link
Member Author

jonaro00 commented Jun 9, 2023

The 'no such file' errors seem quite unaffected by the sleeps.
All I know is that they originate from some error in cargo-generate::generate.

@paulotten
Copy link
Contributor

@jonaro00 is this the most user intuitive approach? How will the user know which git urls they should be using?

Would expanding the list of templates from

Shuttle works with a range of web frameworks. Which one do you want to use?
›
❯ actix-web
  axum
  rocket
  tide
  tower
  poem
  salvo
  serenity
  poise
  warp
  thruster
  none

to something like

  actix-web
  actix-web/postgres
  actix-web/websocket-actorless
  axum
  axum/static-files
  axum/static-next-server
  axum/websocket
  axum/with-state
  ...

be more intuitive?

I suppose you could do both...

@jonaro00
Copy link
Member Author

I like the idea, but there are some things to consider.

  1. Most people that run init in interactive mode probably want to start from a hello world template(?)
  2. Having all templates is more complex, more information overload, harder to see variety in what is available (but fuzzy find helps).
  3. Would the list of examples be hardcoded or dynamically fetched?

I think there can definitely be more hints for how to use the git option, in examples repo and in docs.

@joshua-mo-143
Copy link
Member

joshua-mo-143 commented Jun 13, 2023

What about maybe just adding a dialogue option to ask if the user wants to use a template, then if they do the CLI can just list from a list of templates that uses that framework (with a default of No).

Then if they do actually want a template, it can just get generated from whatever is required.

@jonaro00
Copy link
Member Author

Sounds good. First you choose framework and then between hello-world (default) and other templates (if those exist).

@ivancernja
Copy link
Member

@jonaro00 I agree with @joshua-mo-143

And I also think this is a great idea.

Btw. from your last message; there should be a step asking for whether the users wants to pick a template (y/N) rather than prompt them with the list of templates and have them choose 'No' there.

This is running on the assumption that the majority will be satisfied with the hello world one to start building on top of it.

@jonaro00 jonaro00 changed the title feat: shuttle init --git feat: shuttle init --from Jun 15, 2023
@jonaro00
Copy link
Member Author

Caveat: the now forced git init will init a git repo even when already inside one.

@joshua-mo-143
Copy link
Member

Hmmm.

What if we check the folder to see if there's a .git subfolder? It shouldn't be too difficult.

@jonaro00
Copy link
Member Author

jonaro00 commented Jun 15, 2023

It would need to be a recursive check, but a git lib can probably to this with one line.
EDIT: Yep, that was easy.

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.

Thanks @jonaro00 ! This is pretty useful. It is pretty lengthy too, but there are lots of tests so I am happy. 😃 I still need more time for code review and manual testing, hope you can bear with us for some time. Left few questions for now.

Comment on lines 313 to 343
ActixWeb => vec![
"actix-web/hello-world",
"actix-web/postgres",
"actix-web/websocket-actorless",
],
Axum => vec![
"axum/hello-world",
"axum/static-files",
"axum/static-next-server",
"axum/websocket",
"axum/with-state",
],
Poem => vec!["poem/hello-world", "poem/mongodb", "poem/postgres"],
Poise => vec!["poise/hello-world"],
Rocket => vec![
"rocket/hello-world",
"rocket/authentication",
"rocket/dyn_template_hbs",
"rocket/persist",
"rocket/postgres",
"rocket/secrets",
"rocket/url-shortener",
"rocket/workspace",
],
Salvo => vec!["salvo/hello-world"],
Serenity => vec!["serenity/hello-world", "serenity/postgres"],
Thruster => vec!["thruster/hello-world", "thruster/postgres"],
Tide => vec!["tide/hello-world", "tide/postgres"],
Tower => vec!["tower/hello-world"],
Warp => vec!["warp/hello-world"],
None => vec!["custom/none"],
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I see the usefulness of such a list but how do we decide what is worthy to be included here (e.g url-shortener vs request-scheduler)?

  2. Do we think we'll be adding an example here for all Shuttle resources down the line? Would it be more different if we show somehow the link to the shuttle-examples repo, the Shuttle resources docs, and how to init from a repo if needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I included all examples that are related to the listed frameworks. But yeah, some rule to follow would be good, I'd use this as a first version though.
  2. The link is shown, see screenshot at top. After init is cloning, the examples readme will be updated with proper instructions, and perhaps a growing list of community templates.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we have 2) I would refrain from adding more templates than the hello-world examples. I am not seeing the brought value, it creates a precedent for people, and then including other templates without clear guidelines and purpose is hard (all the templates in the list reiterate documentation/articles more or less - they showcase how to use resources on multiple frameworks or how to create a custom service).

Copy link
Member Author

Choose a reason for hiding this comment

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

The value is that users who are unfamiliar with Shuttle or the examples repo get a better view of what is being offered from the official examples. If they follow the link they can learn more about which templates are offered, but also how to clone any template and check out the community list.
You also wont need to remember/copy+paste the name of common templates that you use ("Let me make another axum app with postgres" -> easy peasy, no lookup of repo name and path).

Copy link
Contributor

@iulianbarbu iulianbarbu Jun 19, 2023

Choose a reason for hiding this comment

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

If they follow the link they can learn more about which templates are offered, but also how to clone any template and check out the community list.

This is sufficient.

You also wont need to remember/copy+paste the name of common templates that you use ("Let me make another axum app with postgres" -> easy peasy, no lookup of repo name and path).

I see the point, but at the same time, it can be an init --from <github_repo_url> which would make it much faster than going through the entire init flow.

. If they follow the link they can learn more about which templates are offered, but also how to clone any template and check out the community list.

We already tell them how to do it when initing (you show up that HINT). I would rather defend against introducing uncertainty and not add learning material in the CLI (makes me think I am doing a lab).

Also, what about initing a template with an older cargo-shuttle than the deps from the repo?
Also, why showing up templates for multiple frameworks with postgres (for example)? Users might think each framework has different requirements. If we add only one then, they'll whether the resource is supported only for a framework? After which point they'll look out to the docs :) I would avoid all these qs and any future ones related to which other templates to add to the CLI (I guarantee that if we add an sqlite one, we would one a template for it, and the same for dynamo-db, and then for s3, and the list goes one until you have a build config system similar to the Linux kernel.

Let's keep it simple stupid from an init perspective and rely on the docs for any learning aspect. The DevX is very important for us, but I am not sure though about this list of templates.

#[tokio::test]
async fn non_interactive_basic_init() {
let temp_dir = Builder::new().prefix("basic-init").tempdir().unwrap();
// Sleep to give time for the directory to finish creating
tokio::time::sleep(tokio::time::Duration::from_millis(500)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

So the exit of the tempdir() is not equivalent with the existence of a temp file on the filesystem you say?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was getting 'no such directoy' error when debugging sporadic failing tests. This was an attempt to track them down, but I left them in to be sure. The tests' run duration don't suffer from this.
I think the errors happen internally in cargo-generate, they also create and delete tmp dirs.

I can remove the sleeps if wanted but they don't hurt either. CI has improved a good bit on this branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to avoid the sleeps by working with a global tempdir, that gets dropped after all tests run. They run in serial order and the ScopedWorkingDirectory Drop impl seems only to rely on setting the env::current_dir when dropped. Not sure why is this.. but hopefully it will not affect how the testing goes. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange! I see a warning about early drops of the temp_dir in the tempfile docs, but it doesn't seem like .path().to_owned() will drop it. This is a bit of a shot in the dark, but could it be because several of the tests use the same name for the temp dir? Either way I think we can merge with these sleeps, but it would be good to be certain that they are required.

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.

Looks great to me! I left a few small nits, mostly from refactoring away from multiple templates. There is also a question about those sleeps I think we should answer, but I'd still say we can merge this tomorrow if we can't figure that out.

cargo-shuttle/src/lib.rs Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
cargo-shuttle/src/lib.rs Outdated Show resolved Hide resolved
#[tokio::test]
async fn non_interactive_basic_init() {
let temp_dir = Builder::new().prefix("basic-init").tempdir().unwrap();
// Sleep to give time for the directory to finish creating
tokio::time::sleep(tokio::time::Duration::from_millis(500)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

Strange! I see a warning about early drops of the temp_dir in the tempfile docs, but it doesn't seem like .path().to_owned() will drop it. This is a bit of a shot in the dark, but could it be because several of the tests use the same name for the temp dir? Either way I think we can merge with these sleeps, but it would be good to be certain that they are required.

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! Just a few comments. We're good to go once we clarify them.

cargo-shuttle/src/args.rs Outdated Show resolved Hide resolved
#[tokio::test]
async fn non_interactive_basic_init() {
let temp_dir = Builder::new().prefix("basic-init").tempdir().unwrap();
// Sleep to give time for the directory to finish creating
tokio::time::sleep(tokio::time::Duration::from_millis(500)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

We might be able to avoid the sleeps by working with a global tempdir, that gets dropped after all tests run. They run in serial order and the ScopedWorkingDirectory Drop impl seems only to rely on setting the env::current_dir when dropped. Not sure why is this.. but hopefully it will not affect how the testing goes. What do you think?

Comment on lines +12 to +18
// DEBUG CI (no such file): SLEEP AND TRY AGAIN?
println!(
"Did not find directory: {} !!! because {:?}",
working_directory, e
);
tokio::time::sleep(tokio::time::Duration::from_millis(500)).await;
canonicalize(working_directory).unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, a bit strange. Why would canonicalize fail on the CI? Would like to see it fail to debug a bit. Can you put back the expect (it was an unwrap before, but it is a bit more user-friendly to use expect)?

@jonaro00
Copy link
Member Author

Regarding all the sleeps and extra steps in tests:

I think the fails stem from that our tests and cargo-generate create a lot of tempdirs in a short amount of time. Not sure though.
One time got an error where the canonicalize failed for the init path (default) argument.
That was ultra rare compared to the more common error: an unwrap somewhere in cargo-generate due to "no such file or dir". Again, probably the tempdirs failing for some reason.
I find the "same random dir name" scenario too unlikely for it to fail so often, but a sequential approach could be taken. Still, this might just randomly fail aswell.

What I can say is that CI holds a bit better due to the changes, and now I'm too afraid to remove any of them 😅.
(If the sleeps are a concern due to CI time, then you are looking at the wrong kinds of tests.)
I'll implement you tips and then you ran run the CI a bunch if u want.

@jonaro00
Copy link
Member Author

Oops, forgot to post this:

The flow now looks like this. I opted to put the examples hint after choosing template because the amount of text combined with the choice filled many lines and pushed text off-screen.

Is the "See the docs or help page." too unclear? (should there also be a link~?)

image

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.

It looks like they're failing every run now since the tests expected output is out of date with the updated hints.

And yes it does seem impossible that using the same prefix would be an issue, I didn't consider the fact that tempfile also adds a random string to the dir name 🙈

Thanks for the screenshot! I think the output looks good, but perhaps add a docs link or extend it to "... or run cargo shuttle init --help? Current way looks good to me too though!

test1/.gitignore Outdated Show resolved Hide resolved
@oddgrd oddgrd merged commit 73cf246 into shuttle-hq:main Jun 20, 2023
@jonaro00 jonaro00 deleted the feat/shuttle-init-git branch June 20, 2023 09:50
@jonaro00 jonaro00 added the S label Jun 20, 2023
@oddgrd oddgrd added M and removed S labels Jun 20, 2023
AlphaKeks pushed a commit to AlphaKeks/shuttle that referenced this pull request Jun 20, 2023
* Refactor init args

* fix test + details

* Polish run integration test

* Add test-case. Try to prevent errors, still not error-free.

* Prompt for other templates

* better help

* Use more of the cargo-generate features, rename args

* bump lockfile

* fix git init

* comments

* Hint serenity users about idle

* remove prompt for list of all templates, improve hints

* not 1.70 too soon

* feedback cleanup

* fix tests

* Better hint

* fix: remove init project

---------

Co-authored-by: oddgrd <29732646+oddgrd@users.noreply.github.com>
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
6 participants