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

Allow direct mounting directory 'pattern' (plus improve errors) #1488

Merged
merged 2 commits into from May 17, 2023

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented May 15, 2023

Fixes #1487 and #1356. This PR does the following two things

Direct mounting directory "patterns"

When the files option is a directory that exists and --direct-mounts is passed, we now allow mapping to happen (previously this was an error). Essentially this makes files = ["some/directory"] equivalent to files = { source = "some/directory", destination = "some/directory" }.

Better error messages

The error message when using a file or glob pattern together with --direct-mounts was the following:

Error: Failed to prepare configuration

Caused by:
    this component cannot be run with `--direct-mount` because it uses file patterns

This was very confusing especially when using a file path and not glob pattern.

The error message now changes to the following in these various situations:

Glob pattern

Error: Failed to prepare configuration

Caused by:
    0: `--direct-mount` flag cannot be used for this spin application
    1: only directories can be directly mounted but attempted to mount the glob pattern 'my-pattern*'

Existing file path

Error: Failed to prepare configuration

Caused by:
    0: `--direct-mount` flag cannot be used for this spin application
    1: only directories can be directly mounted but attempted to mount the file path 'some/file.txt'

Non-existing directory or file path

Error: Failed to prepare configuration

Caused by:
    0: `--direct-mount` flag cannot be used for this spin application
    1: only directories can be directly mounted but attempted to mount the non-existing path 'does/not/exist'

@rylev rylev requested a review from dicej May 15, 2023 08:59
Signed-off-by: Ryan Levick <ryan.levick@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.

Thanks for this - love the improvement, just some nits on the implementation.

crates/loader/src/local/assets.rs Outdated Show resolved Hide resolved
crates/loader/src/local/assets.rs Outdated Show resolved Hide resolved
host: path.absolutize()?.into(),
})
} else {
let typ = if p.contains('*') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an extremely complicated clause, especially at the "an else inside a match inside a map" level of indentation - by the time I get to the end of it I have long since forgotten what I'm doing. Consider pulling out a function e.g. direct_mount_pattern_error(...), or even (maybe better) pulling out the whole match arm (starting on line 79) into direct_mount_for_pattern().

Or even pulling out the whole map body...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I pulled out the map body into its own function. I hope that that helps!

Copy link
Contributor

Choose a reason for hiding this comment

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

IT HELPS SO MUCH

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev requested a review from itowlson May 16, 2023 08:13
@rylev rylev merged commit aecbb29 into main May 17, 2023
9 checks passed
@rylev rylev deleted the improve-file-mounting branch May 17, 2023 12:30
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.

Confusing error when manifest files option includes path and --direct-mount is used
2 participants