-
Notifications
You must be signed in to change notification settings - Fork 239
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
chore: Refactor spin up
OCI support
#1230
Conversation
let s = PathBuf::from(dir).join("logs"); | ||
return Some(s); | ||
} else { | ||
tracing::info!("Unable to retrieve SPIN_STATE_DIR environment variable to persist logs",); |
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.
@michelleN FYI I felt OK removing this because the logic will change significantly with #1175
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.
Delighted to see this: farewell, --from-registry
, you will not be missed. All looks great except for one concern about the default log directory, but probably that's me missing something...?
let loader = TriggerLoader::new(working_dir, self.allow_transient_write); | ||
self.build_executor(loader, locked_url).await? | ||
}; | ||
let loader = TriggerLoader::new(working_dir, self.allow_transient_write); |
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.
YESSSSSSSSSSSSSSSSSS
|
||
fn content_ref(path: impl AsRef<Path>) -> Result<ContentRef> { | ||
let path = std::fs::canonicalize(path)?; | ||
let url = Url::from_file_path(path).map_err(|_| anyhow!("couldn't build file URL"))?; |
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.
Not context
to preserve the underlying error? Or with_context
to show the path that failed (since I don't think the caller adds any context)? Not that it should ever happen of course!
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.
Url::from_file_path
returns Result<Url, ()>
, so there is no underlying error (why? I do not know). Anyway, it shouldn't be possible to fail because of canonicalize
but I wasn't quite confident enough to unwrap
/expect
.
.await | ||
.context("cannot pull Spin application from registry")?; | ||
|
||
// Read locked app |
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 don't think you use the path or content again after this section - suggest extracting a read_locked_app
function
4e3f504
to
5bb14ed
Compare
The main change here is to prepare the file mounts before calling the trigger executor, like local and bindle apps do. Signed-off-by: Lann Martin <lann.martin@fermyon.com>
5bb14ed
to
7e59575
Compare
The main change here is to prepare the file mounts before calling the trigger executor, like local and bindle apps do.