Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
145 changes: 145 additions & 0 deletions docs/issues/drafts/add-explicit-rule-for-atomic-ansible-playbooks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
# Draft Issue: Add Explicit Rule for Atomic Ansible Playbooks

## Problem

During implementation of issue #292, the contributor (AI agent) initially added storage creation tasks directly into existing playbooks (`deploy-compose-files.yml` and `deploy-grafana-provisioning.yml`), violating the Single Responsibility Principle.

The AGENTS.md has 20+ rules, but the atomic playbook principle was implicit rather than explicit. Rule #8 explains _how_ to register playbooks but doesn't state that each playbook should do exactly one thing.

### Root Causes

1. **Rule Density**: Too many rules to remember, easy to miss implicit conventions
2. **Pattern Recognition Failure**: Contributor saw existing playbooks and assumed adding tasks was acceptable
3. **Implicit vs Explicit**: The architecture principle wasn't stated as a clear rule

## Example of the Mistake

### Incorrect Approach (What Was Done Initially)

```yaml
# deploy-compose-files.yml - WRONG: Adding unrelated task
- name: Deploy Docker Compose files
hosts: all
become: true
vars_files:
- variables.yml

tasks:
- name: Create MySQL storage directory # WRONG - Different responsibility!
ansible.builtin.file:
path: "{{ deploy_dir }}/storage/mysql/data"
state: directory
owner: "999"
group: "999"
when: mysql_enabled | default(false)

- name: Copy docker-compose files # Original task
# ...
```

### Correct Approach (After User Correction)

```yaml
# create-mysql-storage.yml - RIGHT: Atomic playbook with single responsibility
- name: Create MySQL storage directory
hosts: all
become: true
vars_files:
- variables.yml

tasks:
- name: Create MySQL data directory with correct ownership
ansible.builtin.file:
path: "{{ deploy_dir }}/storage/mysql/data"
state: directory
mode: "0755"
owner: "999"
group: "999"
when: mysql_enabled | default(false)
```

With conditional execution in Rust:

```rust
// In release handler - conditional check happens in Rust, not Ansible
fn create_mysql_storage(environment: &Environment<Releasing>, ...) {
// Check if MySQL is configured (via tracker database driver)
if !environment.context().user_inputs.tracker().uses_mysql() {
info!("MySQL not configured - skipping storage creation");
return Ok(());
}

// Run the atomic playbook
CreateMysqlStorageStep::new(ansible_client).execute()
}
```

## Proposed Solution

### 1. Update Rule #8 in AGENTS.md

Change from:

```markdown
8. **When adding new Ansible playbooks**: Read `docs/contributing/templates/tera.md`...
```

To:

```markdown
8. **When adding new Ansible playbooks**: Read `docs/contributing/templates/ansible.md` for the complete guide.
- **CRITICAL: One playbook = One responsibility** (Single Responsibility Principle)
- Each playbook should perform exactly ONE conceptual operation
- Conditional logic for service enablement belongs in Rust code, NOT in playbooks with `when:` clauses
- Static playbooks must be registered in `copy_static_templates()`
```

### 2. Add "Red Flags" Section to Ansible Guide

In `docs/contributing/templates/ansible.md`:

```markdown
## Red Flags (Stop and Reconsider)

If you find yourself doing any of these, STOP and reconsider:

- **Adding tasks to an existing playbook** → Create a new atomic playbook instead
- **Using `when:` for service enablement** → Move the condition to Rust code
- **Playbook names with "and"** (e.g., "create-storage-and-deploy-config.yml") → Split into separate playbooks
- **Multiple unrelated `ansible.builtin.file` tasks** → Each belongs in its own playbook

**Correct pattern**: New feature = New atomic playbook + New Rust step with conditional check
```

### 3. Create ADR for Atomic Playbook Architecture

Create `docs/decisions/atomic-ansible-playbooks.md` explaining:

- Why we use atomic playbooks (testability, composability, clear failure points)
- How conditional execution works (Rust checks service enablement, then calls atomic playbook)
- The three-level architecture (Command → Step → Ansible playbook)

### 4. Pre-Implementation Checklist

Add to ansible guide:

```markdown
## Before Adding Ansible Functionality Checklist

- [ ] Am I adding tasks to an existing playbook? → **Create new atomic playbook**
- [ ] Does my playbook do more than one conceptual thing? → **Split it**
- [ ] Am I using `when:` for "should this service run"? → **Move to Rust conditional**
- [ ] Have I created a corresponding Rust step file?
- [ ] Have I registered the playbook in `copy_static_templates()`?
```

## Files to Modify

1. `AGENTS.md` - Update Rule #8
2. `docs/contributing/templates/ansible.md` - Add Red Flags section and checklist
3. Create `docs/decisions/atomic-ansible-playbooks.md` - ADR

## Related

- Issue #292 where this problem was discovered
- Existing pattern: `create-prometheus-storage.yml`, `create-tracker-storage.yml`
72 changes: 72 additions & 0 deletions src/application/command_handlers/release/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,14 @@ pub enum ReleaseCommandHandlerError {
#[error("Prometheus storage creation failed: {0}")]
PrometheusStorageCreation(String),

/// Grafana storage directory creation failed
#[error("Grafana storage creation failed: {0}")]
GrafanaStorageCreation(String),

/// `MySQL` storage directory creation failed
#[error("MySQL storage creation failed: {0}")]
MysqlStorageCreation(String),

/// Caddy configuration deployment failed
#[error("Caddy configuration deployment failed: {0}")]
CaddyConfigDeployment(String),
Expand Down Expand Up @@ -115,6 +123,12 @@ impl Traceable for ReleaseCommandHandlerError {
"ReleaseCommandHandlerError: Prometheus storage creation failed - {message}"
)
}
Self::GrafanaStorageCreation(message) => {
format!("ReleaseCommandHandlerError: Grafana storage creation failed - {message}")
}
Self::MysqlStorageCreation(message) => {
format!("ReleaseCommandHandlerError: MySQL storage creation failed - {message}")
}
Self::CaddyConfigDeployment(message) => {
format!(
"ReleaseCommandHandlerError: Caddy configuration deployment failed - {message}"
Expand Down Expand Up @@ -144,6 +158,8 @@ impl Traceable for ReleaseCommandHandlerError {
| Self::TrackerStorageCreation(_)
| Self::TrackerDatabaseInit(_)
| Self::PrometheusStorageCreation(_)
| Self::GrafanaStorageCreation(_)
| Self::MysqlStorageCreation(_)
| Self::CaddyConfigDeployment(_)
| Self::ReleaseOperationFailed { .. } => None,
}
Expand All @@ -159,6 +175,8 @@ impl Traceable for ReleaseCommandHandlerError {
| Self::TrackerStorageCreation(_)
| Self::TrackerDatabaseInit(_)
| Self::PrometheusStorageCreation(_)
| Self::GrafanaStorageCreation(_)
| Self::MysqlStorageCreation(_)
| Self::CaddyConfigDeployment(_) => ErrorKind::TemplateRendering,
Self::Deployment { .. } | Self::ReleaseOperationFailed { .. } => {
ErrorKind::InfrastructureOperation
Expand Down Expand Up @@ -354,6 +372,54 @@ Common causes:
- Ansible playbook not found
- Network connectivity issues

For more information, see docs/user-guide/commands.md"
}
Self::GrafanaStorageCreation(_) => {
"Grafana Storage Creation Failed - Troubleshooting:

1. Verify the target instance is reachable:
ssh <user>@<instance-ip>

2. Check that the instance has sufficient disk space:
df -h

3. Verify the Ansible playbook exists:
ls templates/ansible/create-grafana-storage.yml

4. Check Ansible execution permissions

5. Review the error message above for specific details

Common causes:
- Insufficient disk space on target instance
- Permission denied on target directories
- Ansible playbook not found
- Network connectivity issues

For more information, see docs/user-guide/commands.md"
}
Self::MysqlStorageCreation(_) => {
"MySQL Storage Creation Failed - Troubleshooting:

1. Verify the target instance is reachable:
ssh <user>@<instance-ip>

2. Check that the instance has sufficient disk space:
df -h

3. Verify the Ansible playbook exists:
ls templates/ansible/create-mysql-storage.yml

4. Check Ansible execution permissions

5. Review the error message above for specific details

Common causes:
- Insufficient disk space on target instance
- Permission denied on target directories
- Ansible playbook not found
- Network connectivity issues

For more information, see docs/user-guide/commands.md"
}
Self::CaddyConfigDeployment(_) => {
Expand Down Expand Up @@ -517,6 +583,12 @@ mod tests {
}),
ReleaseCommandHandlerError::StatePersistence(RepositoryError::NotFound),
ReleaseCommandHandlerError::TemplateRendering("test".to_string()),
ReleaseCommandHandlerError::TrackerStorageCreation("test".to_string()),
ReleaseCommandHandlerError::TrackerDatabaseInit("test".to_string()),
ReleaseCommandHandlerError::PrometheusStorageCreation("test".to_string()),
ReleaseCommandHandlerError::GrafanaStorageCreation("test".to_string()),
ReleaseCommandHandlerError::MysqlStorageCreation("test".to_string()),
ReleaseCommandHandlerError::CaddyConfigDeployment("test".to_string()),
ReleaseCommandHandlerError::DeploymentFailed {
message: "test".to_string(),
source: DeployComposeFilesStepError::ComposeBuildDirNotFound {
Expand Down
Loading
Loading