Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve spin watch #1341

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Conversation

calebschoepp
Copy link
Collaborator

@calebschoepp calebschoepp commented Mar 31, 2023

Changes made

  • Add a README to the example I added
  • More accurate description text for spin watch command
  • Remove all the complicated flags and only offer a --skip-build flag to support the non-default use case i.e. you want to run build yourself
  • Remove TODO item for live reconfiguring watcher. I spent the afternoon spiking this and it was really messy and made the code super hard to read. I'm not convinced it is worth adding this until people start asking for it.

Example use cases

spin watch

  • Watches what you've configured in component.build.watch and your component.files
  • Runs both build and up

spin watch --skip-build

  • Watches your component.source and your component.files
  • Runs only up

@calebschoepp calebschoepp requested review from lann and itowlson March 31, 2023 23:03
@michelleN michelleN added this to the 1.1.0 milestone Apr 3, 2023
Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

I'm wondering if I just haven't quite clicked into the mental model of the watch settings because it's not yet quite clear to me how these options fit together. I'm wondering what the impact would be of expressing it in terms of "what changes do I want to cause a restart" (assets, Wasm, source code) - would it be clearer or more confusing? (Like the "skip-build" setting is I think saying I don't want source code changes to cause a restart. Except that "watch-wasm" seems to be saying that too.)

(But I am also a bit vague on skip-up so I guess it's not just that.)

examples/static-fileserver/README.md Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
@calebschoepp calebschoepp requested a review from itowlson April 4, 2023 21:54
src/commands/watch.rs Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Show resolved Hide resolved
@calebschoepp
Copy link
Collaborator Author

@itowlson I pushed some fixes for all the comment threads we had going and I'm ready for re-review.

Copy link
Contributor

@itowlson itowlson left a comment

Choose a reason for hiding this comment

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

Thanks! A few nits/suggestions but nothing that needs to block getting this into 1.1 - happy to revisit in future polishing/refactoring passes. Looks good!

src/commands/watch.rs Outdated Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
@@ -119,7 +111,10 @@ impl WatchCommand {
return Ok::<(), Infallible>(());
}

// TODO: Check if spin.toml changed and reconfigure
if event.paths().any(|(p, _)| p.ends_with("spin.toml")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should know the path of the manifest; use that instead of the literal text "spin.toml"

src/commands/watch.rs Show resolved Hide resolved
src/commands/watch.rs Outdated Show resolved Hide resolved
Signed-off-by: Caleb Schoepp <caleb.schoepp@fermyon.com>
@calebschoepp calebschoepp merged commit 63337eb into fermyon:main Apr 12, 2023
@calebschoepp calebschoepp deleted the improve-spin-watch branch March 4, 2024 15:37
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants