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

add --direct-mounts option to spin up #967

Merged
merged 3 commits into from
Dec 14, 2022
Merged

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Dec 12, 2022

This option tells spin up to directly mount any asset directories specified in the manifest file instead of copying the contents to a temporary directory and mounting that. This is useful when iteratively developing the static assets in the app, since any modifications to assets are immediately visible to subsequent invocations -- no restart required.

Closes #964.

Signed-off-by: Joel Dice joel.dice@fermyon.com

@dicej dicej requested a review from itowlson December 12, 2022 22:01
This option tells `spin up` to directly mount any asset directories specified in
the manifest file instead of copying the contents to a temporary directory and
mounting that.  This is useful when iteratively developing the static assets in
the app, since any modifications to assets are immediately visible to subsequent
invocations -- no restart required.

Closes fermyon#964.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Copy link
Member

@michelleN michelleN left a comment

Choose a reason for hiding this comment

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

Manually tested this using tests/build/assets-test. I removed the excluded files line from the spin.toml, used the new flag, and then changed the text in the assets to see the change and it worked!!! 🎉

I did see an initial lag the first few times I updated the text but that might have been a vscode thing. Not sure. It'd be nice to have a separate test with assets and no exclusions already set up somewhere so we can manually (or with a testing framework) test this scenario more easily.

tracing::info!("directly mounting local asset directories into guest");

if !exclude_files.is_empty() {
bail!("exclusions not permitted when mounting directories directly")
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean that hidden files will also be mounted?

Copy link
Member

Choose a reason for hiding this comment

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

Separately, should we allow exclusions for this feature in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line means that if you ask for direct mounts for an app that specifies on or more files to be excluded, we'll raise an error and exit without running the app.

Per Ivan's comment, we'll need a more general "hot reload" solution to handle exclusions correctly. It will probably involve using a temporary directory and simultaneously monitoring the original directories and files, copying the non-excluded items over to the temporary directory as they change.

host: source.clone(),
}),
RawFileMount::Pattern(_) => Err(anyhow!(
"patterns not permitted when mounting directories directly"
Copy link
Member

Choose a reason for hiding this comment

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

Should we permit patterns in the future for this feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think that's doable using WASI preopens -- not sure how much work would be required, though. As I mentioned above, a general-purpose "hot reload" feature could handle this using a dynamically-updated temporary directory.

Personally, I prefer the tidiness of directory mounts anyway: you designate a directory for assets, put the ones you need in it (possibly as part of spin build) and omit the ones you don't. You don't need to fuss with patterns or exclusions that way.

Signed-off-by: Joel Dice <joel.dice@fermyon.com>
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.

The functionality looks fine, but I have a few code organisation/formatting requests, and some suggestions on how to frame the "you can't do that here" error messages. Otherwise good to go!

crates/loader/src/local/assets.rs Show resolved Hide resolved
crates/loader/src/local/assets.rs Outdated Show resolved Hide resolved
crates/loader/src/local/assets.rs Outdated Show resolved Hide resolved
crates/loader/src/local/assets.rs Outdated Show resolved Hide resolved
src/commands/up.rs Outdated Show resolved Hide resolved
src/commands/up.rs Outdated Show resolved Hide resolved
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
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.

Looks great now - thanks!

@dicej dicej merged commit 03a74fd into fermyon:main Dec 14, 2022
@dicej dicej deleted the direct-mounts branch December 14, 2022 20:48
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.

Add spin up option to mount static asset directory directly
4 participants