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

fix: don't panic on missing shorebird.yaml #18

Merged
merged 6 commits into from
May 16, 2023
Merged

Conversation

bryanoltman
Copy link
Contributor

@bryanoltman bryanoltman commented May 16, 2023

Description

Fixes shorebirdtech/shorebird#413

Validated by:

  • Creating a new Flutter project
  • Running shorebird init
  • Removing shorebird.yaml
  • Verifying that shorebird --local-engine-src-path=$HOME/Shorebird/engine/src --local-engine=android_release_arm64 run -- --release doesn't panic and still launches the app.

I also tested the release-patch flow to ensure that didn't break.

  • Updates with_config in library/src/config.rs to
    • Return anyhow::Result<R> instead of R
    • Not panic if the config object isn't initialized
    • Return an error in the case where the config object isn't initialized
  • Updates consumers of with_config to handle the possibility that with_config may return an error by propagating the error, usually to log_on_error in library/src/c_api.rs because the updater can't do much if no shorebird.yaml is provided.

Potential Future Work

  • Improve use of anyhow::Error
    • Wrapping our error types in anyhow::Error::from() is pretty verbose and I suspect there's a better, more idiomatic way to use anyhow::Error. The report_launch_result_with_no_current_patch test is especially ugly.

Type of Change

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Copy link
Contributor

@eseidel eseidel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let chat on vc quick.

library/src/c_api.rs Outdated Show resolved Hide resolved
library/src/config.rs Outdated Show resolved Hide resolved
library/src/updater.rs Outdated Show resolved Hide resolved
}

/// Report that the current active path failed to launch.
/// This will mark the patch as bad and activate the next best patch.
pub fn report_launch_failure() -> Result<(), UpdateError> {
pub fn report_launch_failure() -> anyhow::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UpdateError was a dream. Probably can die now that we've teched up to anyhow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #19 – I think we'll want to overhaul our error handling more generally as part of this.

library/src/c_api.rs Outdated Show resolved Hide resolved
library/src/config.rs Outdated Show resolved Hide resolved
// Send info from app + current slot to server.
let response_result = send_patch_check_request(&config, &state);
match response_result {
Err(err) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a follow up patch, we should just make the Rust API return these errors, and the c_api responsible to log them when needed.

@bryanoltman bryanoltman merged commit 11d8c22 into main May 16, 2023
@bryanoltman bryanoltman deleted the bo/config-init-fail branch May 16, 2023 20:21
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.

fix: shorebird engine should not crash if shorebird.yaml is missing
2 participants