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

[Improvement]: Bacon live reload on local runs #1967

Open
1 task done
jonaro00 opened this issue Jan 21, 2025 · 8 comments
Open
1 task done

[Improvement]: Bacon live reload on local runs #1967

jonaro00 opened this issue Jan 21, 2025 · 8 comments
Labels
Contribution Wanted The community is welcome to collaborate on this issue Good First Issue Good for newcomers S-Accepted This will be worked on T-Improvement Improvement or addition to existing features

Comments

@jonaro00
Copy link
Member

Describe the improvement

Continuation of #496

Add an easy way to run the complicated bacon "one liner" to the local run.

Implementation options:

  • Including bacon as a library (?)
  • Call bacon binary if it is installed. Otherwise show instructions/docs link.

Syntax options:

  • shuttle watch
  • shuttle run --watch
  • shuttle run --bacon / shuttle run --use-bacon

I would pick calling the binary separately to not bloat cargo-shuttle with a huge dependency, and calling it shuttle run --bacon to highlight that it's an optional addon, and not built-in.

Duplicate declaration

  • I have searched the issues and this improvement has not been requested before.
@jonaro00 jonaro00 added Contribution Wanted The community is welcome to collaborate on this issue Good First Issue Good for newcomers S-Investigation This issue needs further investigation or design to figure out a solution T-Improvement Improvement or addition to existing features labels Jan 21, 2025
@c-git
Copy link
Contributor

c-git commented Jan 22, 2025

While preparing the update to the instructions I realized that including as a library may not be an appropriate option. I'm not an expert on licences but bacon's licence is AGPL-3.0 and shuttle uses Apache-2.0 so I'm not sure library use would be permitted.

@jonaro00
Copy link
Member Author

Then we can try adding it as shuttle run --bacon, where it uses bacon --version to check that it exists and is at least v3.8.0. If it doesn't, print a link to the local run docs page, if it does, run the "one-liner".

@jonaro00 jonaro00 added S-Accepted This will be worked on and removed S-Investigation This issue needs further investigation or design to figure out a solution labels Jan 27, 2025
@c-git
Copy link
Contributor

c-git commented Jan 27, 2025

I like that plan, seems good to me. I like that it makes it clear that it's calling a 3rd party app. I also like that it's not required or opinionated. It leaves it open to support another tool if one comes up by not using --watch for this one. Watchexec (also made by the same person who made cargo watch) is an alternative but bacon is more customized for rust.

@c-git
Copy link
Contributor

c-git commented Jan 27, 2025

I was thinking it over and realized there seems to be one issue. What about arguments to shuttle run? If we run the same one liner command every time we don't make it easy for the user to pass arguments to shuttle. I see a few approaches that we could take.

  1. Ignore the problem and let users run bacon themselves if they don't want the default (Doesn't seem great if we are trying to make it easier)
  2. Try to see what other arguments were passed and pass them to the bacon invocation as well.
  3. Have the --bacon option accept optional <ARGS> that get appended to the shuttle run invocation we pass to bacon.

I'm personally partial to (2). I think (3) is easier to implement but I feel it's less intuitive for a user of the shuttle binary. (2) allows a user to get everything working as they like in terms of options for shuttle run and then just add --bacon to have the same thing repeated on change.

@jonaro00
Copy link
Member Author

Thought about that as well. I think --bacon can just start off supporting shuttle run without args. If they want custom args in a custom bacon job, they can construct that themselves.

@c-git
Copy link
Contributor

c-git commented Jan 27, 2025

I wouldn't mind trying to implement it at the same time. It may actually not be that hard and there aren't many so testing should be easy. If it proves to be complicated we can always abandon it after we try.

@jonaro00
Copy link
Member Author

Sure, go for it! :D

@c-git
Copy link
Contributor

c-git commented Jan 28, 2025

Ok thanks. Might not have time to do it this week but I'll try to get started soon and open a Draft PR once I do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Contribution Wanted The community is welcome to collaborate on this issue Good First Issue Good for newcomers S-Accepted This will be worked on T-Improvement Improvement or addition to existing features
Projects
None yet
Development

No branches or pull requests

2 participants