[Stacked] Schneems/download bundler struct layer #326
Conversation
The `Changed` struct limits us to a single reason for a change. By using a vec we can list all changes. There will be some redundancy in the output if multiple layers are keyed to the same values (such as distro name or distro version). That might be okay, or we could try storing common information in its own layer up front, and printing that information once (the specific values) and then later we could say "distro version changed" rather than "distro verision changed (22.04 to 24.04)".
Changing the logging will be it's own commit.
The explicit check if the old and new metadata structs were equal isn't needed since I'm already comparing each individual element and have logic to return `None` if they're the same.
Prefer format!(path.display) over `to_string_lossy`.
There was a problem hiding this comment.
This PR looks good -- I've added a few comments/suggestions regarding the log output.
On that note: The cache layer reuse/clearing log output could benefit from a more consistent style (as there are currently 3+ different variations of this across the ruby, bundler and gems layers), e.g.:
- Ruby version `3.1.3` from `default`
- Clearing cache due to change(s): CPU architecture (`amd64` to `arm64`)
- Installing .... (1.6s)
- Bundler version `2.4.17` from `Gemfile.lock`
- Clearing cache Bundler version (`2.4.5` to `2.4.17`)
- Running `gem install bundler --version 2.4.17` ... (0.4s)
- Bundle install
- Clearing cache (cpu architecture changed: amd64 to arm64)
I'm guessing this is already on your radar -- to be clear, I'm not asking you to change stuff related to anything except the bundler layer in this PR :)
It'd also be great if you could add some integration tests that verify the intended log output (that at least cover the output with a clear cache, and after rebuilding and reusing the case). E.g. something like this https://github.com/heroku/buildpacks-dotnet/blob/main/buildpacks/dotnet/tests/sdk_installation_test.rs#L47-L77 . Whenever log output is changed, I like to also see a change in an integration test (or a failed integration test) :)
| } else { | ||
| State::BundlerVersionChanged(old.version, version) | ||
| Some(format!( | ||
| "Bundler version ({old} to {now})", |
There was a problem hiding this comment.
Use lower-case (like the "invalid metadata" cause) https://github.com/heroku/buildpacks-ruby/pull/326/files#diff-cd4ee747be3abfb0ab939cdf3087fa781357327a5eabb3d870f73cc4625b7160R38 and consistent with the output prior to this PR - unless this is intentional/desired ("Bundler" could work with the edit I made above, wrapping the cause in parenthesis). Also added "changed" for context.
| "Bundler version ({old} to {now})", | |
| "bundler version changed ({old} to {now})", |
| EmptyLayerCause::NewlyCreated => {} | ||
| EmptyLayerCause::InvalidMetadataAction { cause } | ||
| | EmptyLayerCause::RestoredLayerAction { cause } => { | ||
| bullet = bullet.sub_bullet(format!("Clearing cache {cause}")); |
There was a problem hiding this comment.
The log output was changed in this PR, to include more details when the bundler version changed, but that also made the log output a bit awkward:
Clearing cache Bundler version (`2.4.5` to `2.4.17`)
The cause also has differing casing (Bundler version vs invalid metadata).
| bullet = bullet.sub_bullet(format!("Clearing cache {cause}")); | |
| bullet = bullet.sub_bullet(format!("Clearing cache {}", style::details(cause))); |
This (combined with the edit below, fixing the bundler version change cause) is one way this could be addressed. Might make sense to consider a different phrasing though as these changes result in:
Clearing cache (bundler version changed (`2.4.5` to `2.4.17`))
| } else { | ||
| ( | ||
| RestoredLayerAction::KeepLayer, | ||
| "using cached version".to_string(), |
There was a problem hiding this comment.
It might make sense to update this to specifically mention "Bundler" here. This is not necessary as part of this PR (it could also be a separate PR if you like), but all the other layers specify what's being reused (e.g. metrics agent, Ruby version, gems:
- Metrics agent
- Using cached metrics agent
- Ruby version `3.1.3` from `default`
- Using cached Ruby version
- Bundler version `2.4.17` from `Gemfile.lock`
- Using cached version
- Bundle install
- Loading cached gems
| "using cached version".to_string(), | |
| "using cached Bundler version".to_string(), |
|
Going to close this and start fresh after landing #327. I took a lot of this feedback and channeled it into that PR (re consistency and testing). |
No description provided.