-
Notifications
You must be signed in to change notification settings - Fork 9
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(cli): muti-typegraph bug + logging polish #717
fix(cli): muti-typegraph bug + logging polish #717
Conversation
WalkthroughThe updates encompass multiple areas: platform migration from "aarch64-darwin" to "x86_64-linux" in Changes
These changes collectively enhance platform compatibility, dependency management, error handling, and logging throughout the codebase. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
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.
Actionable comments posted: 32
Out of diff range and nitpick comments (6)
meta-cli/src/deploy/actors/console/input.rs (1)
89-89
: Ensure consistent logging levels across the application.Consider using a consistent logging level for user interactions. If
log::error
is used here, ensure it aligns with the logging strategy elsewhere in the application.meta-cli/src/deploy/push/migration_resolution.rs (1)
Line range hint
106-117
: Handle errors more gracefully in asynchronous operations.- console.error(err.to_string()); - panic!("{err:?}"); // could occur if the latest migration does not match + if let Err(e) = console.error(err.to_string()).await { + error!("Failed to log error: {e}"); + } + error!("Migration failed with error: {err}");This change avoids using
panic!
, which can abruptly terminate the application, and instead logs the error appropriately.meta-cli/src/typegraph/loader/mod.rs (2)
Line range hint
75-96
: Consider handling the case whereModuleType::try_from
fails.- ModuleType::try_from(path.as_path()).unwrap(), + ModuleType::try_from(path.as_path()).map_err(|e| LoaderError::InvalidModuleType { path: path.clone(), error: e })?,This change ensures that the function does not panic if the conversion fails, instead it returns an appropriate error.
Line range hint
192-346
: Add error handling forCommand::new
inget_load_command
.- let mut command = Command::new(argv[0]); + let mut command = Command::new(argv[0]).map_err(|e| LoaderError::CommandCreationFailed { command: argv[0].to_string(), error: e })?;This ensures that errors during command creation are handled and reported properly.
meta-cli/src/cli/deploy.rs (2)
Line range hint
106-150
: Ensure consistent error handling in the deployment initialization.- .context("error while building node from config")?; + .map_err(|e| anyhow!("Error while building node from config: {e}"))?;This change standardizes the error handling to use a more descriptive message, improving the clarity of the error context.
Line range hint
163-276
: Refactor the deployment run logic to reduce complexity.- if !self.options.allow_dirty { - let repo = git2::Repository::discover(&deploy.config.base_dir).ok(); - if let Some(repo) = repo { - let dirty = repo.statuses(None)?.iter().any(|s| !s.status().is_empty() && !s.status().contains(git2::Status::IGNORED)); - if dirty { - bail!("Dirty repository not allowed"); - } - } else { - warn!("Not in a git repository."); - } - } + self.ensure_clean_repository(&deploy)?;Extract the repository cleanliness check into a separate method to simplify the
run
method and improve code readability.
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.
lgtm
94f28ca
to
2f6e2e0
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
meta-cli/src/typegraph/loader/mod.rs (1)
[!TIP]
Codebase VerificationThe instances of
load_module
inmeta-cli/src/deploy/actors/loader.rs
need to be updated to match the new asynchronous setup and return type ofLoaderResult
.
- File:
meta-cli/src/deploy/actors/loader.rs
- Lines:
fn load_module(&self, self_addr: Addr<Self>, path: Arc<PathBuf>) { ... }
match loader.load_module(path.clone()).await { ... }
self.load_module(ctx.address(), msg.0);
self.load_module(ctx.address(), msg.0);
Please ensure these calls are updated to handle the asynchronous nature and the return type of
LoaderResult
.Analysis chain
Line range hint
75-96
: LGTM! Ensure all function calls match the new setup.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `load_module` match the new setup. # Test: Search for the function usage. Expect: Only occurances of the new setup. rg --type rust $'load_module'Length of output: 513
Script:
#!/bin/bash # Extract lines around the instances of `load_module` in `meta-cli/src/deploy/actors/loader.rs` to verify the function calls. rg --type rust -A 5 -B 5 $'load_module' meta-cli/src/deploy/actors/loader.rsLength of output: 1466
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.
Actionable comments posted: 0
Out of diff range and nitpick comments (2)
typegate/tests/runtimes/wasm_wire/rust/build.sh (2)
Line range hint
5-5
:pushd
is not POSIX compliant.Replace
pushd ..
withcd ..
.- pushd .. + cd ..
Line range hint
7-7
:popd
is not POSIX compliant.Replace
popd
withcd -
.- popd + cd -
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.
Actionable comments posted: 1
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #717 +/- ##
==========================================
+ Coverage 74.05% 74.06% +0.01%
==========================================
Files 120 120
Lines 13777 13776 -1
Branches 1407 1407
==========================================
+ Hits 10202 10203 +1
+ Misses 3549 3547 -2
Partials 26 26 ☔ View full report in Codecov by Sentry. |
@coderabbitai based on the patterns established in typegate/tests/e2e/cli/dev_test.ts and typegate/tests/e2e/website/website_test.ts, generate a new test file inside typegate/tests/e2e/cli/ that directory that has tests for deployment of multiple typegraphs at once. Name a few typegraphs from the examples directory and specify them using the |
Tip For best results, initiate chat on the files or code changes. New Test File:
|
72db3af
to
2e6ba48
Compare
a1b8c25
into
feat/met-533-fix-typegate-logging
- Fixes multi-typegraph deployment bug. - Fixes issue with loader process existing after cli death. - Replaces `anyhow` with `color-eyre` in cli and improved logging story. - `interlude` pattern was used to minimize changes by wrapping `eyre` through an `anyhow` export. - [ ] Tests MET-516 MET-517 MET-477 <!-- 5. Readiness checklist - [ ] The change come with new or modified tests - [ ] Hard-to-understand functions have explanatory comments - [ ] End-user documentation is updated to reflect the change --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> - **Bug Fixes** - Corrected typos in configuration files to ensure proper authentication settings. - **New Features** - Enhanced error handling with improved context and logging for better debugging. - **Refactor** - Updated dependencies and imports across multiple modules for consistency and performance. - Reorganized error handling to use `eyre` library for more descriptive error messages. - **Style** - Standardized logging and error message formats for clarity and uniformity. - **Chores** - Updated platform compatibility in configuration files. - Removed deprecated dependencies and added new ones to improve functionality and maintainability. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
anyhow
withcolor-eyre
in cli and improved logging story.interlude
pattern was used to minimize changes by wrappingeyre
through ananyhow
export.MET-516
MET-517
MET-477
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
eyre
library for more descriptive error messages.Style
Chores