Skip to content

Conversation

@marekzan
Copy link
Owner

This pull request includes extensive changes to the veno project, focusing on renaming modules, refactoring code, and improving the configuration and notification system. The most important changes include renaming the cli and core modules, refactoring the AppConfig and Artifact structures, and enhancing the notification system.

Module Renaming:

  • Renamed cli to veno-cli and core to veno-core in Cargo.toml and updated paths accordingly. [1] [2] [3]

Refactoring:

  • Removed the old AppConfig and Artifact structures and their associated methods from core/src/config.rs and core/src/artifact/mod.rs. [1] [2]
  • Introduced new AppState structure in veno-core/src/app.rs to handle application state and configuration loading.
  • Refactored AppConfig to a simpler structure in veno-core/src/config.rs.
  • Updated Artifact structure and methods in veno-core/src/artifact/mod.rs.

Notification System Enhancements:

  • Added artifact_ids to the Notifier structure to link notifiers to specific artifacts.
  • Refactored the send method in Sink implementations to use a consistent notification parameter. [1] [2]
  • Implemented new notification generation logic in AppState to handle artifact version checks and notifications.

Dependency Management:

  • Added anyhow and tokio as workspace dependencies in Cargo.toml.
  • Updated dependencies in veno-cli/Cargo.toml and veno-core/Cargo.toml to use workspace versions. [1] [2]

Miscellaneous:

  • Renamed core/src/lib.rs to veno-core/src/lib.rs and updated module imports. [1] [2]
  • Made minor adjustments to the EmailSink and SlackSink implementations. [1] [2]

These changes collectively improve the structure, maintainability, and functionality of the veno project.

- make requests concurrent
- rename modules
- simplify core api
@marekzan marekzan requested a review from Copilot April 12, 2025 20:06
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 27 out of 27 changed files in this pull request and generated 1 comment.

Comment on lines +28 to +42
fn replace_env_placeholders(config: &mut String) {
let result = RE_ENV.replace_all(config, |caps: &Captures| {
// caps[0] is "${VAR}"
// caps[1] is "VAR"
let var_name = &caps[1];
// TODO is there a better way to handle this? It would be better to bubble this up since it is not recoverable
match env::var(var_name) {
Ok(val) => val,
Err(_) => panic!("Could not find env.var = {}", &caps[0]),
}
});

if let Cow::Owned(new_content) = result {
*config = new_content;
}
Copy link

Copilot AI Apr 12, 2025

Choose a reason for hiding this comment

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

Consider returning a Result error instead of panicking when an environment variable is missing so that the error can be handled gracefully. This would allow the caller to manage the failure without an abrupt termination.

Suggested change
fn replace_env_placeholders(config: &mut String) {
let result = RE_ENV.replace_all(config, |caps: &Captures| {
// caps[0] is "${VAR}"
// caps[1] is "VAR"
let var_name = &caps[1];
// TODO is there a better way to handle this? It would be better to bubble this up since it is not recoverable
match env::var(var_name) {
Ok(val) => val,
Err(_) => panic!("Could not find env.var = {}", &caps[0]),
}
});
if let Cow::Owned(new_content) = result {
*config = new_content;
}
fn replace_env_placeholders(config: &mut String) -> Result<()> {
let result = RE_ENV.replace_all(config, |caps: &Captures| {
// caps[0] is "${VAR}"
// caps[1] is "VAR"
let var_name = &caps[1];
match env::var(var_name) {
Ok(val) => val,
Err(_) => return Err(anyhow::anyhow!("Missing environment variable: {}", &caps[0])),
}
});
if let Cow::Owned(new_content) = result {
*config = new_content;
}
Ok(())

Copilot uses AI. Check for mistakes.
@marekzan marekzan merged commit 2416e5d into main Apr 12, 2025
1 check passed
@marekzan marekzan deleted the feature/simplify_api branch April 12, 2025 20:12
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