Skip to content

Commit

Permalink
Fix shared layer logic (#364)
Browse files Browse the repository at this point in the history
There was a bug reported where deploying a Ruby application after modifying the Gemfile.lock would result in an error because we were skipping running `bundle install`. Debugging this lead me to find that we were returning the incorrect value from our shared cache logic (accidentally returning the currnt/now value rather than the old value).

The idea behind `cached_layer_write_metadata` is to either return the old cached data or return a message stating why it couldn't be returned. It was accidentally returning the new/current value instead.

I added a unit test to assert this behavior. While developing I used an integration test to debug the issue (given below).

This commit fixes the integration test given below:

## Gemfile.lock cache invalidation integration reproduction


```
$ cargo test test_gemfile_lock_invalidates_cache -- --exact
```

```
#[test]
fn test_gemfile_lock_invalidates_cache() {
    let app_dir = tempfile::tempdir().unwrap();
    fs_err::write(
        app_dir.path().join("Gemfile"),
        r#"
        source "https://rubygems.org"

        gem "rake"
    "#,
    )
    .unwrap();

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.0)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();
    let mut config = amd_arm_builder_config("heroku/builder:24", &app_dir.path().to_string_lossy());
    TestRunner::default().build(
        config.clone()
        .buildpacks([
            BuildpackReference::CurrentCrate,
        ]),
        |context| {
            let stdout = context.pack_stdout.clone();
            println!("{}", stdout);
            assert_contains!(stdout, "# Heroku Ruby Buildpack");
            assert_contains!(
                stdout,
                r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
            );
            assert_contains!(stdout, "Installing rake");

    fs_err::write(
        app_dir.path().join("Gemfile.lock"),
        r"
GEM
  remote: https://rubygems.org/
  specs:
    rake (13.2.1)

PLATFORMS
  ruby

DEPENDENCIES
  rake
",
    )
    .unwrap();

            context.rebuild(
                config.buildpacks([BuildpackReference::CurrentCrate]),
                |rebuild_context| {
                    println!("{}", rebuild_context.pack_stdout);

                assert_contains!(
                    rebuild_context.pack_stdout,
                    r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#
                );
                assert_contains!(rebuild_context.pack_stdout, "Installing rake");
                },
            );
        });
}
```
  • Loading branch information
schneems authored Dec 11, 2024
1 parent a3f5834 commit 51f0399
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 2 deletions.
4 changes: 4 additions & 0 deletions buildpacks/ruby/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Fixed

- A bug introduced in 4.0.0 would result in incorrectly skipping running `bundle install` when the `Gemfile` or `Gemfile.lock` or environment variables had changed. The bug is now fixed. ([#364](https://github.com/heroku/buildpacks-ruby/pull/364))

## [4.0.0] - 2024-11-27

### Changed
Expand Down
33 changes: 31 additions & 2 deletions buildpacks/ruby/src/layers/shared.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ pub(crate) trait MetadataDiff {

/// Standardizes formatting for layer cache clearing behavior
///
/// If the diff is empty, there are no changes and the layer is kept
/// If the diff is empty, there are no changes and the layer is kept and the old data is returned
/// If the diff is not empty, the layer is deleted and the changes are listed
pub(crate) fn restored_layer_action<M>(old: &M, now: &M) -> (RestoredLayerAction, Meta<M>)
where
M: MetadataDiff + Clone,
{
let diff = now.diff(old);
if diff.is_empty() {
(RestoredLayerAction::KeepLayer, Meta::Data(now.clone()))
(RestoredLayerAction::KeepLayer, Meta::Data(old.clone()))
} else {
(
RestoredLayerAction::DeleteLayer,
Expand Down Expand Up @@ -222,6 +222,35 @@ mod tests {
}
migrate_toml_chain! {TestMetadata}

#[test]
fn test_restored_layer_action_returns_old_data() {
#[derive(Debug, Clone)]
struct AlwaysNoDiff {
value: String,
}

impl MetadataDiff for AlwaysNoDiff {
fn diff(&self, _: &Self) -> Vec<String> {
vec![]
}
}

let old = AlwaysNoDiff {
value: "old".to_string(),
};
let now = AlwaysNoDiff {
value: "now".to_string(),
};

let result = restored_layer_action(&old, &now);
match result {
(RestoredLayerAction::KeepLayer, Meta::Data(data)) => {
assert_eq!(data.value, "old");
}
_ => panic!("Expected to keep layer"),
}
}

#[test]
fn test_cached_layer_write_metadata_restored_layer_action() {
let temp = tempfile::tempdir().unwrap();
Expand Down

0 comments on commit 51f0399

Please sign in to comment.