-
Notifications
You must be signed in to change notification settings - Fork 249
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
Fix spin up --help
for unbuilt apps
#1980
Conversation
Signed-off-by: Lann Martin <lann.martin@fermyon.com>
First we "resolve" the AppSource into a ResolvedAppSource. This step isn't expected to "load" an app fully, e.g. copying static assets. At this stage we have enough information to determine the trigger executor, which lets us serve `--help`. Then we "load" the ResolvedAppSource into a LockedApp as before. Note that this change is basically a no-op for OCI apps as the changes to split these stages are incompatible with the rest of the OCI code today. Signed-off-by: Lann Martin <lann.martin@fermyon.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a nit about naming but it's all internal and we can always noodle on it later - this works great, thanks!
}, | ||
} | ||
|
||
impl ResolvedAppSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm tripping over the name here - too many app sources I fear, and this is a different sense of "source" from the one a few lines above. The use case for this is more "application manifest" - I realise the water is muddied by it fully materialising the OCI case, but given your TODO of only loading the lockfile, maybe we should consider a manifesty name, with the fully resolved OCI case being an implementation detail?
(For me this is probably exacerbated by the "resolved" because of knowing the old name for the "figure out the source from the command" function!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the 3rd unsatisfactory name I came up with.
async fn prepare_app_from_file( | ||
// Take the AppSource and do the minimum amount of work necessary to | ||
// be able to resolve the trigger executor. | ||
async fn resolve_app_source( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(a note for the future, not for now) This function and load_resolved_app_source
now have almost nothing to do with self
- it might be interesting to explore making AppSource and ResolvedAppSource more self-contained at some future point. Not a consideration for now though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed up.rs
seems ripe for major refactoring "pretty soon".
This shuffles the
spin up
logic around a bit to allow--help
calls to execute before fully loading local apps. A nice side benefit is that--help
should be a bit faster, especially for apps with lots offiles
. Note thatOCI
behavior is (should be) unchanged as the current OCI codebase doesn't really allow for splitting up "get the manfiest" and "load everything".Fixes #1957