diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 78fdf22f..8c5f8b13 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -103,6 +103,8 @@ These principles should guide all development decisions, code reviews, and featu 11. **When writing Markdown documentation**: Be aware of GitHub Flavored Markdown's automatic linking behavior. Read [`docs/contributing/github-markdown-pitfalls.md`](../docs/contributing/github-markdown-pitfalls.md) for critical patterns to avoid. **NEVER use hash-number patterns for enumeration or step numbering** - this creates unintended links to GitHub issues/PRs. Use ordered lists or alternative formats instead. +12. **When creating new environment variables**: Read [`docs/contributing/environment-variables-naming.md`](../docs/contributing/environment-variables-naming.md) for comprehensive guidance on naming conventions (condition-based vs action-based), decision frameworks, and best practices. Also review [`docs/decisions/environment-variable-prefix.md`](../docs/decisions/environment-variable-prefix.md) to ensure all project environment variables use the `TORRUST_TD_` prefix for proper namespacing and avoiding conflicts with system or user variables. + ## ๐Ÿงช Build & Test - **Setup Dependencies**: `cargo run --bin dependency-installer install` (sets up required development tools) diff --git a/docs/contributing/README.md b/docs/contributing/README.md index 17b8132a..43b322f3 100644 --- a/docs/contributing/README.md +++ b/docs/contributing/README.md @@ -4,24 +4,25 @@ This guide will help you understand our development practices and contribution w ## ๐Ÿ“‹ Quick Reference -| Topic | File | -| ------------------------------------ | ------------------------------------------------------------ | -| DDD layer placement (architecture) | [ddd-layer-placement.md](./ddd-layer-placement.md) | -| PR review guide for reviewers | [pr-review-guide.md](./pr-review-guide.md) | -| Creating roadmap issues | [roadmap-issues.md](./roadmap-issues.md) | -| Branching conventions | [branching.md](./branching.md) | -| Commit process and pre-commit checks | [commit-process.md](./commit-process.md) | -| Code quality and linting | [linting.md](./linting.md) | -| Module organization and imports | [module-organization.md](./module-organization.md) | -| Error handling principles | [error-handling.md](./error-handling.md) | -| Working with Tera templates | [templates.md](./templates.md) | -| Debugging techniques | [debugging.md](./debugging.md) | -| Spell checking and dictionaries | [spelling.md](./spelling.md) | -| Known issues and expected behaviors | [known-issues.md](./known-issues.md) | -| Logging best practices | [logging-guide.md](./logging-guide.md) | -| GitHub Markdown pitfalls | [github-markdown-pitfalls.md](./github-markdown-pitfalls.md) | -| GitHub Copilot agent firewall | [copilot-agent-firewall.md](./copilot-agent-firewall.md) | -| Testing conventions and practices | [testing/](./testing/) | +| Topic | File | +| ------------------------------------ | -------------------------------------------------------------------- | +| DDD layer placement (architecture) | [ddd-layer-placement.md](./ddd-layer-placement.md) | +| PR review guide for reviewers | [pr-review-guide.md](./pr-review-guide.md) | +| Creating roadmap issues | [roadmap-issues.md](./roadmap-issues.md) | +| Branching conventions | [branching.md](./branching.md) | +| Commit process and pre-commit checks | [commit-process.md](./commit-process.md) | +| Code quality and linting | [linting.md](./linting.md) | +| Module organization and imports | [module-organization.md](./module-organization.md) | +| Error handling principles | [error-handling.md](./error-handling.md) | +| Working with Tera templates | [templates.md](./templates.md) | +| Environment variables naming | [environment-variables-naming.md](./environment-variables-naming.md) | +| Debugging techniques | [debugging.md](./debugging.md) | +| Spell checking and dictionaries | [spelling.md](./spelling.md) | +| Known issues and expected behaviors | [known-issues.md](./known-issues.md) | +| Logging best practices | [logging-guide.md](./logging-guide.md) | +| GitHub Markdown pitfalls | [github-markdown-pitfalls.md](./github-markdown-pitfalls.md) | +| GitHub Copilot agent configuration | [copilot-agent/](./copilot-agent/) | +| Testing conventions and practices | [testing/](./testing/) | ## ๐Ÿš€ Getting Started diff --git a/docs/contributing/copilot-agent/README.md b/docs/contributing/copilot-agent/README.md new file mode 100644 index 00000000..a8e07c7e --- /dev/null +++ b/docs/contributing/copilot-agent/README.md @@ -0,0 +1,33 @@ +# GitHub Copilot Agent Configuration + +This directory contains documentation for configuring and working with GitHub Copilot coding agent in the Torrust Tracker Deployer project. + +## Documents + +### [Firewall Configuration](./firewall.md) + +Describes the firewall configuration for GitHub Copilot coding agent, including: + +- Custom allowlist domains (e.g., `opentofu.org`) +- Recommended allowlist coverage +- Setup instructions for repository administrators + +### [Pre-commit Configuration](./pre-commit-config.md) + +Explains how to configure environment variables to skip slow tests during Copilot agent pre-commit checks: + +- Why this is needed (timeout issues) +- How to set up `TORRUST_TD_SKIP_SLOW_TESTS` environment variable +- Pre-commit timing breakdown (individual tasks and unit test breakdown) +- What gets skipped (E2E tests and coverage checks) +- Testing and verification instructions + +## Related Issues + +- [GitHub Issue #121](https://github.com/torrust/torrust-tracker-deployer/issues/121) - Install Git Pre-Commit Hooks for Copilot Agent +- [Community Discussion](https://github.com/orgs/community/discussions/178998) - Copilot timeout during long-running pre-commit checks + +## References + +- [GitHub Copilot Coding Agent Documentation](https://docs.github.com/en/copilot/using-github-copilot/using-copilot-coding-agent-to-work-on-tasks) +- [Customizing Copilot Agent Environment](https://docs.github.com/en/copilot/how-tos/use-copilot-agents/coding-agent/customize-the-agent-environment) diff --git a/docs/contributing/copilot-agent-firewall.md b/docs/contributing/copilot-agent/firewall.md similarity index 100% rename from docs/contributing/copilot-agent-firewall.md rename to docs/contributing/copilot-agent/firewall.md diff --git a/docs/contributing/copilot-agent/pre-commit-config.md b/docs/contributing/copilot-agent/pre-commit-config.md new file mode 100644 index 00000000..efe768b1 --- /dev/null +++ b/docs/contributing/copilot-agent/pre-commit-config.md @@ -0,0 +1,175 @@ +# Setting up TORRUST_TD_SKIP_SLOW_TESTS for GitHub Copilot Agent + +This document explains how to configure the `TORRUST_TD_SKIP_SLOW_TESTS` environment variable for GitHub Copilot coding agent to skip slow tests during pre-commit checks. + +## Why This Is Needed + +GitHub Copilot coding agent has a hardcoded ~5-6 minute timeout for command execution. Our full pre-commit verification (including E2E tests and coverage checks) takes ~5.5 minutes, causing the agent to timeout and retry infinitely. + +**Related Issues:** + +- GitHub Issue: #121 +- Community Discussion: https://github.com/orgs/community/discussions/178998 + +## Solution + +We use an environment variable (`TORRUST_TD_SKIP_SLOW_TESTS=true`) to skip slow tests (E2E tests and code coverage) when Copilot agent runs pre-commit checks. This keeps checks under the timeout limit while maintaining full verification for local development. + +**Note:** The variable name follows action-based naming (describes behavior, not context). For a comprehensive discussion on condition-based vs action-based environment variable naming, see [Environment Variables Naming Guide](../environment-variables-naming.md). All Torrust Tracker Deployer environment variables use the `TORRUST_TD_` prefix as documented in [Environment Variable Prefix ADR](../../decisions/environment-variable-prefix.md). + +## Pre-commit Timing Breakdown + +Understanding where time is spent helps explain why we skip certain tests for the agent: + +### Individual Task Timings + +| Task | Time | % of Total | Category | Skipped in Fast Mode? | +| ------------- | ----------- | ---------- | -------- | --------------------- | +| cargo machete | 0.08s | 0.04% | Instant | โŒ No | +| All linters | 18.75s | 5.7% | Fast | โŒ No | +| Unit tests | 1m 16s | 22.9% | Medium | โŒ No | +| cargo doc | 44s | 13.4% | Medium | โŒ No | +| E2E provision | 44s | 13.4% | Medium | โœ… **Yes** | +| E2E config | 48s | 14.4% | Medium | โœ… **Yes** | +| Coverage | 1m 29s | 26.9% | Slowest | โœ… **Yes** | +| **TOTAL** | **~5m 30s** | **100%** | - | - | + +**Fast Mode Total: ~3m 48s** (31% time reduction, ~2 minute safety margin below timeout) + +### Unit Tests Breakdown (cargo test) + +The unit tests (`cargo test`) complete in **1m 16s** and include: + +| Test Suite | Time | Tests | Description | +| ---------------------- | ----------- | -------- | ------------------------------------ | +| Unit tests (lib) | 12.24s | 1200 | Core library unit tests | +| e2e_create_command | 13.45s | 4 | End-to-end create command workflow | +| e2e_destroy_command | 0.65s | 4 | End-to-end destroy command workflow | +| file_lock_multiprocess | 6.05s | 8 | Multi-process file locking tests | +| logging_integration | 0.13s | 11 | Logging system integration tests | +| ssh_client_integration | 11.31s | 9 | SSH client integration tests | +| template_integration | 0.01s | 4 | Template rendering integration tests | +| Doc tests | 15.44s | 289 | Documentation example tests | +| **TOTAL** | **~1m 16s** | **1529** | All test suites | + +**Key Insight:** Even though unit tests take 1m 16s (22.9% of total), they're **NOT skipped** in fast mode because: + +- They validate correctness across the entire codebase +- Many are fast unit tests (12.24s for 1200 tests) +- Integration tests provide critical coverage +- Doc tests ensure documentation examples work + +**What We Skip:** E2E tests (1m 32s) and coverage (1m 29s) are skipped because: + +- They're the slowest checks (54.8% of total time combined) +- E2E tests run in CI workflows after PR creation +- Coverage runs in CI and provides informational metrics +- Skipping them provides ~3 minute time savings + +## How to Configure + +### Step 1: Navigate to Repository Settings + +1. Go to your GitHub repository: https://github.com/torrust/torrust-tracker-deployer +2. Click **Settings** (requires admin access) +3. In the left sidebar, click **Environments** + +### Step 2: Configure the Copilot Environment + +1. Click on the `copilot` environment (it should already exist) +2. Scroll down to **Environment variables** +3. Click **Add environment variable** + +### Step 3: Add the Variable + +1. **Name**: `TORRUST_TD_SKIP_SLOW_TESTS` +2. **Value**: `true` +3. Click **Add variable** + +## How It Works + +### For Local Development (Full Verification) + +When developers run `./scripts/pre-commit.sh` locally: + +- `TORRUST_TD_SKIP_SLOW_TESTS` is **not set** (defaults to `false`) +- All checks run, including E2E tests and coverage (~5.5 minutes) +- Full quality verification maintained + +### For Copilot Agent (Fast Verification) + +When Copilot agent runs the pre-commit hook: + +- `TORRUST_TD_SKIP_SLOW_TESTS=true` is injected from the `copilot` environment +- E2E tests and coverage checks are **skipped** (~3.8 minutes total) +- CI workflows will still run all tests after the PR is created + +## Verification + +To verify the configuration is working: + +1. Check that the variable exists in Settings > Environments > copilot +2. Wait for Copilot agent to create a PR +3. Check the session logs - you should see: `โš ๏ธ Running in fast mode (skipping slow tests)` + +## What Gets Skipped + +When `TORRUST_TD_SKIP_SLOW_TESTS=true`: + +**Skipped (saves ~3 minutes):** + +- โŒ E2E provision and destroy tests (~44s) +- โŒ E2E configuration tests (~48s) +- โŒ Code coverage check (~1m 29s) + +**Still runs (maintains quality):** + +- โœ… Cargo machete - unused dependencies (~0.08s) +- โœ… All linters - markdown, YAML, Rust, shellcheck (~19s) +- โœ… Unit tests - 1529 tests across all suites (~1m 16s) +- โœ… Cargo documentation build (~44s) + +**Total time: ~3m 48s** (vs ~5m 30s in full mode) + +## CI Safety Net + +Even though slow tests are skipped in pre-commit for Copilot agent, they still run: + +- In GitHub Actions workflows on PR creation +- In the full CI pipeline before merging + +This ensures no regressions slip through while keeping Copilot agent functional. + +## Running Skipped Tests Manually + +The pre-commit script will inform you about skipped tests and provide commands to run them: + +**For AI agents:** You can run these commands separately (each completes in < 5 minutes): + +```bash +# Run E2E provision tests (~44s) +cargo run --bin e2e-provision-and-destroy-tests + +# Run E2E config tests (~48s) +cargo run --bin e2e-config-tests + +# Run coverage check (~1m 29s) +cargo cov-check +``` + +**For developers:** Test the fast mode locally: + +```bash +TORRUST_TD_SKIP_SLOW_TESTS=true ./scripts/pre-commit.sh +``` + +To run the full verification locally (default): + +```bash +./scripts/pre-commit.sh +``` + +## References + +- [GitHub Docs: Setting environment variables in Copilot's environment](https://docs.github.com/en/copilot/how-tos/use-copilot-agents/coding-agent/customize-the-agent-environment#setting-environment-variables-in-copilots-environment) +- [Community Discussion: Copilot timeout issue](https://github.com/orgs/community/discussions/178998) diff --git a/docs/contributing/environment-variables-naming.md b/docs/contributing/environment-variables-naming.md new file mode 100644 index 00000000..f206fecb --- /dev/null +++ b/docs/contributing/environment-variables-naming.md @@ -0,0 +1,510 @@ +# Environment Variable Naming: Condition vs Action + +This guide explains when to use condition-based vs action-based naming for environment variables, with practical examples and decision-making frameworks. + +## The Fundamental Question + +When naming environment variables, you face a choice: + +- **Condition-based** (describes context/state): `RUNNING_IN_AGENT_ENV`, `IS_PRODUCTION`, `IN_CI` +- **Action-based** (describes behavior/effect): `SKIP_SLOW_TESTS`, `ENABLE_DEBUG`, `USE_CACHE` + +**There is no universal "always use X" rule** - the best choice depends on the **scope of impact** and **purpose** of the variable. + +## Two Schools of Thought + +### Condition-Based Naming + +**Philosophy**: Describe _what is happening_ or _where you are_ + +**Examples:** + +```bash +NODE_ENV=production +RAILS_ENV=development +CI=true +RUNNING_IN_DOCKER=true +IS_PREVIEW_ENVIRONMENT=true +``` + +**Characteristics:** + +- Declarative style: "I am X" +- Describes the system's state or context +- Often triggers multiple behavioral changes +- Common for platform/infrastructure concerns +- Application logic decides what to do with the condition + +**Code pattern:** + +```rust +if env::var("RUNNING_IN_AGENT_ENV").unwrap_or_default() == "true" { + // Agent has time constraints + skip_slow_tests = true; + reduce_logging = true; + disable_interactive_prompts = true; +} +``` + +### Action-Based Naming + +**Philosophy**: Describe _what effect you want_ or _what behavior to change_ + +**Examples:** + +```bash +SKIP_SLOW_TESTS=true +ENABLE_DEBUG_LOGGING=true +USE_PRODUCTION_DATABASE=true +DISABLE_TELEMETRY=true +FORCE_COLOR_OUTPUT=true +``` + +**Characteristics:** + +- Imperative style: "Do X" +- Describes the intended behavior directly +- Usually controls one specific behavior +- Common for feature toggles +- Clear, single-purpose intent + +**Code pattern:** + +```rust +if env::var("SKIP_SLOW_TESTS").unwrap_or_default() == "true" { + // Very clear: skip slow tests + skip_slow_tests = true; +} +``` + +## Decision Framework + +Use this decision tree to choose the right approach: + +```text +Does this variable affect multiple subsystems? +โ”œโ”€ YES โ†’ Consider Condition-Based +โ”‚ โ””โ”€ Example: ENVIRONMENT=staging affects logging, DB, cache, API endpoints +โ”‚ +โ””โ”€ NO โ†’ Prefer Action-Based + โ””โ”€ Example: SKIP_VALIDATION=true only affects validation logic + +Is this a platform/infrastructure concern? +โ”œโ”€ YES โ†’ Condition-Based +โ”‚ โ””โ”€ Example: CI=true, IS_DOCKER=true, KUBERNETES_SERVICE_HOST +โ”‚ +โ””โ”€ NO โ†’ Action-Based + โ””โ”€ Example: ENABLE_FEATURE_X=true, USE_CACHE=true + +Do multiple developers/systems need to interpret this differently? +โ”œโ”€ YES โ†’ Condition-Based (let each decide behavior) +โ”‚ โ””โ”€ Example: NODE_ENV=production โ†’ bundler optimizes, logger reduces verbosity +โ”‚ +โ””โ”€ NO โ†’ Action-Based (explicit intent) + โ””โ”€ Example: COMPRESS_RESPONSES=true โ†’ clear, unambiguous +``` + +## Scope-Based Guidelines + +### 1. Platform/Infrastructure Level โ†’ Condition-Based + +**When the environment itself is the concern:** + +```bash +# Good: Describes where the application is running +ENVIRONMENT=production +NODE_ENV=development +RAILS_ENV=test +KUBERNETES_SERVICE_HOST=10.0.0.1 +CI=true +``` + +**Why condition-based?** + +- Multiple subsystems need to know the context +- Different components may react differently +- Standard conventions (NODE_ENV, RAILS_ENV) aid ecosystem compatibility + +**Example in practice:** + +```javascript +// package.json - Multiple tools use NODE_ENV +{ + "scripts": { + "start": "NODE_ENV=production node server.js", + "dev": "NODE_ENV=development nodemon server.js" + } +} + +// Webpack, Babel, Express, etc. all check NODE_ENV and adjust behavior +``` + +### 2. Feature/Behavior Level โ†’ Action-Based + +**When toggling specific functionality:** + +```bash +# Good: Describes what behavior to change +SKIP_SLOW_TESTS=true +ENABLE_DEBUG_LOGGING=true +USE_REDIS_CACHE=true +DISABLE_RATE_LIMITING=true +FORCE_SSL=true +``` + +**Why action-based?** + +- Single responsibility - one variable, one behavior +- Self-documenting - immediately clear what changes +- No ambiguity about intent +- Easier to test - toggle one thing at a time + +**Example in practice:** + +```rust +// Clear, focused behavior control +if env::var("SKIP_SLOW_TESTS").unwrap_or_default() == "true" { + steps.retain(|step| !step.is_slow()); +} + +if env::var("ENABLE_DEBUG_LOGGING").unwrap_or_default() == "true" { + logger.set_level(Level::Debug); +} +``` + +### 3. Hybrid Approach (Real-World Pattern) + +**Many systems use both strategies together:** + +```bash +# Condition (broad context) +NODE_ENV=production + +# Actions (specific overrides) +ENABLE_DEBUG=true # Override: debug even in production +SKIP_MINIFICATION=true # Override: skip minification in production +USE_LOCAL_DATABASE=true # Override: use local DB in production +``` + +**Pattern:** + +- Condition sets defaults for an environment +- Actions provide fine-grained control to override defaults + +## Real-World Examples + +### Example 1: GitHub Actions CI + +**Condition-based:** + +```yaml +env: + CI: true # Condition: "we are in CI" + GITHUB_ACTIONS: true # Condition: "we are in GitHub Actions" +``` + +**Why?** Many tools check `CI=true` and adjust behavior (disable colors, reduce interactivity, etc.) + +### Example 2: Feature Flags + +**Action-based:** + +```bash +ENABLE_NEW_PAYMENT_FLOW=true +ENABLE_EXPERIMENTAL_API=true +ENABLE_BETA_FEATURES=true +``` + +**Why?** Each variable controls one specific feature - clear, testable, manageable. + +### Example 3: Docker Detection (Anti-pattern) + +**โŒ Poor (action-based for context):** + +```bash +SKIP_HOST_NETWORK_CHECK=true # Why are we skipping? Not clear! +``` + +**โœ… Better (condition-based):** + +```bash +RUNNING_IN_DOCKER=true # Context is clear +# Code decides: "if in Docker, skip host network check" +``` + +### Example 4: Torrust Tracker Deployer (This Project) + +**Our choice: Action-based (`TORRUST_TD_SKIP_SLOW_TESTS`)** + +**Rationale:** + +- โœ… Specific behavior toggle (not platform context) +- โœ… Single responsibility (skip slow tests only) +- โœ… Clear intent (no guessing what happens) +- โœ… Reusable in multiple contexts (agent, local dev, CI) +- โœ… Testable (easy to verify behavior) + +**Alternative we rejected: `TORRUST_TD_RUNNING_IN_AGENT_ENV`** + +**Why rejected:** + +- โŒ Describes context, not intent +- โŒ Code must infer: "if agent env, then what?" +- โŒ Tied to one specific context (agent) +- โŒ Less reusable (can't use for other scenarios) + +## Naming Conventions + +### General Best Practices + +1. **Use SCREAMING_SNAKE_CASE** + + - `ENVIRONMENT`, not `environment` or `Environment` + - Industry standard across all languages + +2. **Be explicit and descriptive** + + - โœ… `ENABLE_REQUEST_LOGGING` + - โŒ `LOG` (too vague) + +3. **Use prefixes for namespacing** + + - โœ… `TORRUST_TD_SKIP_SLOW_TESTS` + - Prevents conflicts with system/library variables + +4. **Boolean-like variables should be obvious** + + - โœ… `ENABLE_X=true/false` + - โœ… `SKIP_X=true/false` + - โœ… `IS_X=true/false` + - โŒ `X=1` (unclear what 1 means) + +5. **Avoid negative logic when possible** + - โœ… `ENABLE_CACHE=false` (clear) + - โŒ `DISABLE_CACHE=false` (double negative) + - Exception: When the default is "enabled" and you want to disable + +### Action-Based Naming Patterns + +Common prefixes for action-based variables: + +- `ENABLE_*` - Turn on a feature +- `DISABLE_*` - Turn off a feature +- `SKIP_*` - Skip an operation +- `USE_*` - Use a specific implementation/resource +- `FORCE_*` - Override normal behavior +- `ALLOW_*` - Permission-related +- `REQUIRE_*` - Requirement-related + +**Examples:** + +```bash +ENABLE_DEBUG_MODE=true +DISABLE_TELEMETRY=true +SKIP_MIGRATIONS=true +USE_MOCK_DATA=true +FORCE_HTTPS=true +ALLOW_ANONYMOUS_ACCESS=true +REQUIRE_EMAIL_VERIFICATION=true +``` + +### Condition-Based Naming Patterns + +Common patterns for condition-based variables: + +- `*_ENV` - Environment type +- `IS_*` - Boolean state +- `IN_*` - Location/context +- `RUNNING_IN_*` - Execution context +- `HAS_*` - Capability presence + +**Examples:** + +```bash +NODE_ENV=production +IS_PRODUCTION=true +IN_KUBERNETES=true +RUNNING_IN_CONTAINER=true +HAS_GPU=true +``` + +## Common Anti-Patterns + +### โŒ Ambiguous Names + +```bash +# Bad: What does this do? +MODE=fast +OPTIMIZATION=1 +CONFIG_TYPE=special +``` + +**Fix:** Be explicit about behavior: + +```bash +# Good: Clear what happens +SKIP_SLOW_TESTS=true +ENABLE_OPTIMIZATIONS=true +USE_PRODUCTION_CONFIG=true +``` + +### โŒ Inconsistent Naming + +```bash +# Bad: Mixing styles without reason +IS_PRODUCTION=true +SKIP_TESTS=true +debugMode=enabled +``` + +**Fix:** Choose a consistent pattern: + +```bash +# Good: Consistent style +ENVIRONMENT=production +SKIP_SLOW_TESTS=true +ENABLE_DEBUG_MODE=true +``` + +### โŒ Overly Generic Platform Variables for Specific Behaviors + +```bash +# Bad: Too broad for specific behavior +AGENT_MODE=true # What does this actually do? +``` + +**Fix:** Use action-based for specific behaviors: + +```bash +# Good: Clear, specific behaviors +SKIP_SLOW_TESTS=true +REDUCE_LOG_VERBOSITY=true +DISABLE_INTERACTIVE_PROMPTS=true +``` + +### โŒ Coupling Too Many Behaviors to One Condition + +```bash +# Bad: One variable does too much +if env::var("RUNNING_IN_AGENT").is_ok() { + skip_tests(); + disable_logging(); + skip_validation(); + compress_output(); + use_fast_mode(); + disable_colors(); + // ... and 10 more things +} +``` + +**Fix:** Separate concerns or use explicit action variables: + +```bash +# Good: Each behavior is controllable +SKIP_SLOW_TESTS=true +LOG_LEVEL=warn +SKIP_VALIDATION=false +COMPRESS_OUTPUT=true +``` + +## Testing Considerations + +**Action-based variables are easier to test:** + +```rust +#[test] +fn it_should_skip_slow_tests_when_env_var_set() { + std::env::set_var("SKIP_SLOW_TESTS", "true"); + + let steps = get_verification_steps(); + + assert!(!steps.iter().any(|s| s.is_slow())); +} +``` + +**Condition-based requires testing all derived behaviors:** + +```rust +#[test] +fn it_should_adapt_to_agent_environment() { + std::env::set_var("RUNNING_IN_AGENT_ENV", "true"); + + // Must test all behaviors that change + assert!(tests_are_skipped()); + assert!(logging_is_reduced()); + assert!(prompts_are_disabled()); + // ... many more assertions +} +``` + +## Migration Strategy + +If you need to migrate from condition-based to action-based (or vice versa): + +### Step 1: Support Both (Transitional Period) + +```rust +// Support old and new variable names +let skip_slow_tests = env::var("SKIP_SLOW_TESTS").is_ok() + || env::var("RUNNING_IN_AGENT_ENV").is_ok(); +``` + +### Step 2: Deprecation Warning + +```rust +if env::var("RUNNING_IN_AGENT_ENV").is_ok() { + eprintln!("Warning: RUNNING_IN_AGENT_ENV is deprecated. Use SKIP_SLOW_TESTS=true instead."); +} +``` + +### Step 3: Update Documentation + +Document the new variable and migration path. + +### Step 4: Remove Old Variable (After Grace Period) + +After sufficient time (version bumps, etc.), remove support for old variable. + +## Summary and Recommendations + +### Use Condition-Based When: + +- โœ… Describing platform/infrastructure context +- โœ… Multiple subsystems need to know the context +- โœ… Following ecosystem conventions (NODE_ENV, RAILS_ENV) +- โœ… The "environment" itself is the concern + +### Use Action-Based When: + +- โœ… Controlling specific behaviors or features +- โœ… Single-purpose toggles +- โœ… Clear, testable behavior changes +- โœ… Could be reused in multiple contexts +- โœ… User/developer needs explicit control + +### For Torrust Tracker Deployer: + +We chose **action-based** (`TORRUST_TD_SKIP_SLOW_TESTS`) because: + +- It's a specific behavior control (not platform context) +- Single responsibility (one variable, one purpose) +- Reusable across contexts (agent, dev, CI) +- Clear, testable, self-documenting + +### General Principle + +> **"Start action-based (specific), move to condition-based only when multiple behaviors need to coordinate."** + +If you find yourself checking one condition to control many unrelated behaviors, that's a sign you might need multiple action-based variables instead. + +## References + +- [The Twelve-Factor App - Config](https://12factor.net/config) +- [Environment Variables Naming Convention](https://en.wikipedia.org/wiki/Environment_variable#Naming_conventions) +- [Docker Environment Variables Best Practices](https://docs.docker.com/compose/environment-variables/best-practices/) +- [GitHub Actions Environment Variables](https://docs.github.com/en/actions/learn-github-actions/environment-variables) + +## Related Documentation + +- [Environment Variable Prefix ADR](../decisions/environment-variable-prefix.md) - Project naming convention +- [Copilot Agent Pre-commit Config](./copilot-agent/pre-commit-config.md) - Practical example of this decision diff --git a/packages/dependency-installer/README.md b/packages/dependency-installer/README.md index e5ff14a5..74dac7bd 100644 --- a/packages/dependency-installer/README.md +++ b/packages/dependency-installer/README.md @@ -44,7 +44,7 @@ This package can detect and install the following development dependencies: ### GitHub Copilot Agent Requirements -When running in GitHub Copilot agent environment, network access to external domains is restricted by a firewall. The OpenTofu installer requires `opentofu.org` to be added to the custom allowlist. See [Copilot Agent Firewall Configuration](../../docs/contributing/copilot-agent-firewall.md) for details on configured domains and setup instructions. +When running in GitHub Copilot agent environment, network access to external domains is restricted by a firewall. The OpenTofu installer requires `opentofu.org` to be added to the custom allowlist. See [Copilot Agent Firewall Configuration](../../docs/contributing/copilot-agent/firewall.md) for details on configured domains and setup instructions. ## Usage diff --git a/project-words.txt b/project-words.txt index 6accf7d8..d62c76a3 100644 --- a/project-words.txt +++ b/project-words.txt @@ -100,6 +100,7 @@ MVVM myapp myenv nameof +namespacing nanos newgrp newtype diff --git a/scripts/pre-commit.sh b/scripts/pre-commit.sh index 331eff78..c8bd1d38 100755 --- a/scripts/pre-commit.sh +++ b/scripts/pre-commit.sh @@ -15,15 +15,44 @@ set -euo pipefail # - command: The command to execute # ============================================================================ -declare -a STEPS=( - "Checking for unused dependencies (cargo machete)|No unused dependencies found|||cargo machete" - "Running linters|All linters passed|||cargo run --bin linter all" - "Running tests|All tests passed|||cargo test" - "Testing cargo documentation|Documentation builds successfully|||cargo doc --no-deps --bins --examples --workspace --all-features" - "Running E2E provision and destroy tests|Provision and destroy tests passed|(Testing infrastructure lifecycle - this may take a few minutes)|RUST_LOG=warn|cargo run --bin e2e-provision-and-destroy-tests" - "Running E2E configuration tests|Configuration tests passed|(Testing software installation and configuration)|RUST_LOG=warn|cargo run --bin e2e-config-tests" - "Running code coverage check|Coverage meets 75% threshold|(Informational only - does not block commits)||cargo cov-check" -) +# Determine which steps to run based on environment +# When TORRUST_TD_SKIP_SLOW_TESTS=true (set for Copilot agent), skip slow tests to avoid timeout issues +# Slow tests include: E2E tests (~1m 32s) and code coverage (~1m 29s) +if [ "${TORRUST_TD_SKIP_SLOW_TESTS:-false}" = "true" ]; then + echo "โš ๏ธ Running in fast mode (skipping slow tests)" + echo "" + echo "The following tests are SKIPPED to stay within the 5-minute timeout limit:" + echo " โ€ข E2E provision and destroy tests (~44 seconds)" + echo " โ€ข E2E configuration tests (~48 seconds)" + echo " โ€ข Code coverage check (~1 minute 29 seconds)" + echo "" + echo "๐Ÿ’ก These tests will run automatically in CI after PR creation." + echo "" + echo "If you want to run them manually before committing, use these commands:" + echo " cargo run --bin e2e-provision-and-destroy-tests # ~44s" + echo " cargo run --bin e2e-config-tests # ~48s" + echo " cargo cov-check # ~1m 29s" + echo "" + echo "Fast mode execution time: ~3 minutes 48 seconds" + echo "" + + declare -a STEPS=( + "Checking for unused dependencies (cargo machete)|No unused dependencies found|||cargo machete" + "Running linters|All linters passed|||cargo run --bin linter all" + "Running tests|All tests passed|||cargo test" + "Testing cargo documentation|Documentation builds successfully|||cargo doc --no-deps --bins --examples --workspace --all-features" + ) +else + declare -a STEPS=( + "Checking for unused dependencies (cargo machete)|No unused dependencies found|||cargo machete" + "Running linters|All linters passed|||cargo run --bin linter all" + "Running tests|All tests passed|||cargo test" + "Testing cargo documentation|Documentation builds successfully|||cargo doc --no-deps --bins --examples --workspace --all-features" + "Running E2E provision and destroy tests|Provision and destroy tests passed|(Testing infrastructure lifecycle - this may take a few minutes)|RUST_LOG=warn|cargo run --bin e2e-provision-and-destroy-tests" + "Running E2E configuration tests|Configuration tests passed|(Testing software installation and configuration)|RUST_LOG=warn|cargo run --bin e2e-config-tests" + "Running code coverage check|Coverage meets 75% threshold|(Informational only - does not block commits)||cargo cov-check" + ) +fi # ============================================================================ # HELPER FUNCTIONS