Skip to content
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

Closed
wants to merge 9 commits into from

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Sep 9, 2024

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.

- 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).
@schneems schneems force-pushed the schneems/struct-bullet-stream branch from b2a203f to d7588d3 Compare September 16, 2024 18:07
@schneems schneems marked this pull request as ready for review September 16, 2024 18:48
@schneems schneems requested a review from a team as a code owner September 16, 2024 18:48
@schneems schneems enabled auto-merge (squash) September 16, 2024 18:57
Copy link
Contributor

@runesoerensen runesoerensen left a 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:

  1. Migrate to struct layer API.
  2. Migrate to bullet_stream.
  3. 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).

Comment on lines +74 to +110
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();
}
};
Copy link
Contributor

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?

Suggested change
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 { .. } => {
Copy link
Contributor

@runesoerensen runesoerensen Sep 18, 2024

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)

@schneems schneems closed this Sep 23, 2024
auto-merge was automatically disabled September 23, 2024 19:50

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants