Skip to content

Conversation

@glefer
Copy link
Owner

@glefer glefer commented Sep 26, 2025

This pull request refactors and improves the notification and CLI command codebase, focusing on simplifying event structures, enhancing type safety, and improving English language consistency throughout the code and documentation. The key changes include removing unused or redundant event fields, updating event and factory logic to match new data structures, and rewriting CLI command help and comments in English for clarity.

Event model and type improvements:

  • Removed unused or redundant fields from runner events (such as runner_id, techno, techno_version, and restarted) in RunnerStarted, RunnerStopped, and related event classes to simplify the event model.
  • Added new required fields to build and update events, such as duration and image_size for BuildCompleted, and image_name for BuildFailed and UpdateAvailable, to provide more detailed and consistent event data. [1] [2]

Notification factory and event construction:

  • Updated the notification event factory (_iter_events in factory.py) to match the new event structures, including handling new fields and removing obsolete ones, ensuring correct event instantiation from operation results. [1] [2] [3]

CLI command and documentation improvements:

  • Rewrote CLI command docstrings, help, and output messages in English for better clarity and consistency, replacing previous French documentation. [1] [2] [3] Fc33f333L140R140, [4] [5]
  • Removed outdated in-line comments and streamlined notification calls in CLI commands, reflecting the simplified event model and improving readability. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]

General code and documentation cleanup:

  • Translated all module-level and inline comments from French to English for consistency and maintainability. [1] [2]
  • Removed legacy compatibility code and comments from notification channel and event logic, further simplifying the codebase. [1] [2] [3]

These changes collectively modernize the notification system, make event handling more robust, and improve the developer experience by clarifying documentation and reducing unnecessary complexity.

@glefer glefer requested a review from Copilot September 26, 2025 11:10
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.

Pull Request Overview

This pull request refactors and modernizes the notification and CLI command codebase by simplifying event structures, enhancing type safety, and translating all French documentation and comments to English for better international maintainability.

  • Removes redundant event fields (runner_id, techno, restarted) and adds new required fields (duration, image_size, image_name) to event classes
  • Updates the notification event factory to match new simplified event structures and field requirements
  • Translates all French docstrings, comments, and CLI help text to English throughout the codebase

Reviewed Changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/ Removed French comments from test files and updated test data to match new event field requirements
src/services/webhook_service.py Translated all French docstrings and comments to English
src/services/notification_service.py Updated notification methods to use simplified event structures without redundant fields
src/services/docker_service.py Translated docstrings to English and added image size/duration tracking to build operations
src/services/config_service.py Translated docstrings and comments from French to English
src/presentation/cli/commands.py Translated all CLI command help text and docstrings from French to English
src/notifications/ Simplified event classes by removing redundant fields and updated factory logic accordingly

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@glefer glefer requested a review from Copilot September 26, 2025 11:13
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.

Pull Request Overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

src/presentation/cli/commands.py:424

  • The French console messages 'Scheduler arrêté manuellement' and 'Erreur dans le scheduler' should be translated to English to maintain consistency with the PR's documentation changes.
        scheduler_service.start()
    except KeyboardInterrupt:
        console.print("[yellow]Scheduler arrêté manuellement.[/yellow]")
        scheduler_service.stop()
    except Exception as e:
        console.print(f"[red]Erreur dans le scheduler: {str(e)}[/red]")

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

scheduler_service.start.assert_called_once()
scheduler_service.stop.assert_called_once()
console.print.assert_called_once_with(
"[yellow]Scheduler arrêté manuellement.[/yellow]"
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

The message 'Scheduler arrêté manuellement' should be translated to English to maintain consistency with the PR's goal of English language standardization.

Suggested change
"[yellow]Scheduler arrêté manuellement.[/yellow]"
"[yellow]Scheduler stopped manually.[/yellow]"

Copilot uses AI. Check for mistakes.
# Vérifier que les méthodes attendues ont été appelées
scheduler_service.start.assert_called_once()
console.print.assert_called_once_with(
f"[red]Erreur dans le scheduler: {str(test_exception)}[/red]"
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

The message 'Erreur dans le scheduler' should be translated to English to maintain consistency with the PR's goal of English language standardization.

Suggested change
f"[red]Erreur dans le scheduler: {str(test_exception)}[/red]"
f"[red]Error in scheduler: {str(test_exception)}[/red]"

Copilot uses AI. Check for mistakes.
console = Console()

# Sous-commande pour les webhooks
webhook_app = typer.Typer(help="Commandes pour tester et déboguer les webhooks")
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

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

The help text 'Commandes pour tester et déboguer les webhooks' should be translated to English to maintain consistency with the PR's documentation changes.

Suggested change
webhook_app = typer.Typer(help="Commandes pour tester et déboguer les webhooks")
webhook_app = typer.Typer(help="Commands to test and debug webhooks")

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Sep 26, 2025

Coverage

Coverage Report
FileStmtsMissCoverMissing
__init__.py10100% 
notifications
   dispatcher.py130100% 
   events.py820100% 
   factory.py400100% 
notifications/channels
   base.py110100% 
   webhook.py210100% 
presentation
   __init__.py00100% 
presentation/cli
   __init__.py00100% 
   commands.py1740100% 
   main.py190100% 
   webhook_commands.py940100% 
services
   __init__.py40100% 
   config_schema.py830100% 
   config_service.py200100% 
   docker_logger.py220100% 
   docker_service.py4180100% 
   notification_service.py500100% 
   scheduler_service.py1620100% 
   webhook_service.py1570100% 
TOTAL13710100% 

@glefer glefer force-pushed the fix/webhook-dynamisation branch from 17f4153 to 87510de Compare September 29, 2025 05:58
@glefer glefer force-pushed the fix/webhook-dynamisation branch from 4eccaaf to e170fe5 Compare September 29, 2025 10:56
@glefer glefer merged commit 38a7747 into main Sep 29, 2025
2 checks passed
@glefer glefer deleted the fix/webhook-dynamisation branch September 29, 2025 10:58
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