-
Notifications
You must be signed in to change notification settings - Fork 7
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
Migrate to the Struct API and output via bullet_stream
#320
Conversation
- AppCacheCollection is hard to use with bullet stream and it's an extremely thin wrapper anyway. - ConfigureEnvLayer and DefaultEnvLayer are not the way to move forward, especially since we have the "structure" api for layers (versus the previous Trait based API).
b2a203f
to
d7588d3
Compare
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 PR would be faster to review if it introduced the changes in at least 2 PRs instead:
- Migrate to struct layer API.
- Migrate to bullet_stream.
- Handles deprecations in the commons/publicly shared crate (although this could also be done in 2, as it doesn't really have an impact on the review complexity like the other changes do).
match layer.state { | ||
LayerState::Restored { .. } => { | ||
bullet = bullet.sub_bullet("Using cached Ruby version"); | ||
} | ||
LayerState::Empty { | ||
cause: | ||
EmptyLayerCause::InvalidMetadataAction { ref cause } | ||
| EmptyLayerCause::RestoredLayerAction { ref cause }, | ||
} => { | ||
bullet = bullet.sub_bullet(format!("Clearing cache ({cause})")); | ||
} | ||
LayerState::Empty { | ||
cause: EmptyLayerCause::NewlyCreated, | ||
} => {} | ||
}; | ||
|
||
match layer.state { | ||
LayerState::Restored { .. } => {} | ||
LayerState::Empty { .. } => { | ||
let timer = bullet.start_timer("Installing"); | ||
let tmp_ruby_tgz = NamedTempFile::new() | ||
.map_err(RubyInstallError::CouldNotCreateDestinationFile) | ||
.map_err(RubyBuildpackError::RubyInstallError)?; | ||
|
||
let url = download_url(&new_metadata.target_id(), &new_metadata.ruby_version) | ||
.map_err(RubyBuildpackError::RubyInstallError)?; | ||
|
||
download(url.as_ref(), tmp_ruby_tgz.path()) | ||
.map_err(RubyBuildpackError::RubyInstallError)?; | ||
|
||
untar(tmp_ruby_tgz.path(), layer.path()) | ||
.map_err(RubyBuildpackError::RubyInstallError)?; | ||
|
||
layer.write_metadata(new_metadata)?; | ||
bullet = timer.done(); | ||
} | ||
}; |
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.
Might make sense to combine the two match layer.state
blocks?
match layer.state { | |
LayerState::Restored { .. } => { | |
bullet = bullet.sub_bullet("Using cached Ruby version"); | |
} | |
LayerState::Empty { | |
cause: | |
EmptyLayerCause::InvalidMetadataAction { ref cause } | |
| EmptyLayerCause::RestoredLayerAction { ref cause }, | |
} => { | |
bullet = bullet.sub_bullet(format!("Clearing cache ({cause})")); | |
} | |
LayerState::Empty { | |
cause: EmptyLayerCause::NewlyCreated, | |
} => {} | |
}; | |
match layer.state { | |
LayerState::Restored { .. } => {} | |
LayerState::Empty { .. } => { | |
let timer = bullet.start_timer("Installing"); | |
let tmp_ruby_tgz = NamedTempFile::new() | |
.map_err(RubyInstallError::CouldNotCreateDestinationFile) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
let url = download_url(&new_metadata.target_id(), &new_metadata.ruby_version) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
download(url.as_ref(), tmp_ruby_tgz.path()) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
untar(tmp_ruby_tgz.path(), layer.path()) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
layer.write_metadata(new_metadata)?; | |
bullet = timer.done(); | |
} | |
}; | |
match layer.state { | |
LayerState::Restored { .. } => { | |
bullet = bullet.sub_bullet("Using cached Ruby version"); | |
} | |
LayerState::Empty { ref cause } => { | |
match cause { | |
EmptyLayerCause::InvalidMetadataAction { cause } | |
| EmptyLayerCause::RestoredLayerAction { cause } => { | |
bullet = bullet.sub_bullet(format!("Clearing cache ({cause})")); | |
} | |
EmptyLayerCause::NewlyCreated => {} | |
}; | |
let timer = bullet.start_timer("Installing"); | |
let tmp_ruby_tgz = NamedTempFile::new() | |
.map_err(RubyInstallError::CouldNotCreateDestinationFile) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
let url = download_url(&new_metadata.target_id(), &new_metadata.ruby_version) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
download(url.as_ref(), tmp_ruby_tgz.path()) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
untar(tmp_ruby_tgz.path(), layer.path()) | |
.map_err(RubyBuildpackError::RubyInstallError)?; | |
layer.write_metadata(new_metadata)?; | |
bullet = timer.done(); | |
} | |
}; |
} | ||
} | ||
LayerState::Empty { .. } => { |
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 doesn't handle the cases when the metrics agent is updated due to either another metrics agent URL, or invalid metadata (the original code logged these events)
This PR deprecates the
commons::build_output
module in favor of the bullet stream crate. It also uses the updated "struct API" from libcnb.rs. To minimize the change, I tried to avoid refactoring code as much as possible.Future refactors (not implemented)
I would like to eventually move the
layers/*.rs
files out and get rid of that nested directory. It adds nothing but overhead. I would also like to refactor the enum based approach to layer invalidation. It's overly complex and adds little value. Ideally I would like a list of all keys that changed and I experimented with a way to do this automatically via relying on toml comparisons in #203. I think this can be extracted and generalized.