Conversation
dreadnode-renovate-bot
Bot
added
area/github
There was a problem hiding this comment.
Pull request overview
OSS-readiness sweep for DreadGOAD: adds standard community/security metadata, clarifies which tool to use per provider, and refactors AWS region resolution so AWS-scoped CLI commands fail fast when region is unset (instead of silently picking a default).
Changes:
- Added OSS hygiene files and GitHub contribution templates (SECURITY/CODE_OF_CONDUCT/CITATION + issue/PR templates).
- Documentation refresh to clearly separate AWS (
dreadgoad) vs non-AWS (goad.sh/goad.py) workflows, plus a new “CLI vs goad.py” reference page. - Centralized AWS region resolution via
Config.ResolveRegion*()and removed silent default region fallbacks (inventory now returns""when unset).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| SECURITY.md | Adds private vulnerability reporting guidance and scope clarification for “tooling vs intentional lab vulns”. |
| CODE_OF_CONDUCT.md | Adds Contributor Covenant CoC with additional project-specific security-content scope. |
| CITATION.cff | Adds citation metadata for academic/training/report usage. |
| README.md | Clarifies tool/provider split and links to new CLI-vs-goad.py doc. |
| docs/mkdocs/mkdocs.yml | Adds nav entry for the new CLI-vs-goad.py doc page. |
| docs/mkdocs/docs/cli-vs-goadpy.md | New provider×tool matrix and command equivalence guide. |
| docs/mkdocs/docs/usage/index.md | Rewrites usage docs to separate AWS (dreadgoad) vs other providers (goad.sh). |
| docs/mkdocs/docs/providers/aws.md | Updates AWS provider docs to reflect dreadgoad-driven workflow (Terragrunt/Warpgate/SSM). |
| docs/mkdocs/docs/installation/index.md | Updates installation landing page to clarify which tool to use per provider. |
| ansible/requirements.yml | Pins community.general collection and geerlingguy.mysql role versions. |
| ansible/galaxy.yml | Pins community.general dependency range for the collection. |
| .github/PULL_REQUEST_TEMPLATE.md | Adds structured PR template with testing/checklist prompts. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds bug-report issue form (tooling-scoped + required repro fields). |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds feature-request issue form. |
| .github/ISSUE_TEMPLATE/config.yml | Disables blank issues and adds contact links (incl. security reporting). |
| cli/internal/inventory/parser.go | Changes Inventory.Region() to return "" when unset; updates guidance in comments. |
| cli/internal/inventory/parser_test.go | Updates tests to assert missing region returns empty string. |
| cli/internal/config/config.go | Introduces ResolveRegion() / ResolveRegionWithInventory(); updates infra path helpers to error when region missing. |
| cli/internal/ansible/retry.go | Uses centralized region resolution for SSM cleanup/fix paths. |
| cli/cmd/root.go | Updates --region help text to reflect required-when-needed semantics. |
| cli/cmd/lab.go | Removes silent default region fallback; uses ResolveRegion() (errors if unset). |
| cli/cmd/infra.go | Removes silent default region fallback; uses ResolveRegion() (errors if unset). |
| cli/cmd/infra_cmd.go | Removes silent default region fallback across infra commands; uses ResolveRegion(). |
| cli/cmd/inventory.go | Uses ResolveRegionWithInventory() when discovering instances/mapping; improves display when inventory region is unset. |
| cli/cmd/ssm.go | Uses ResolveRegionWithInventory() consistently across ssm subcommands (incl. ssm run). |
| cli/cmd/config_cmd.go | Updates config output / scaffold comments to reflect new region requirements. |
| cli/cmd/ami.go | Adjusts AMI commands to require region for list/purge, but allow template-defined region for build unless overridden. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
@mkultraWasHere Proposed blog post draft: https://github.com/dreadnode/DreadGOAD/blob/main/docs/blog-open-source-release.md |
These two ansible dependencies were previously unpinned, meaning every fresh `ansible-galaxy install` could pick up a different upstream version on different days. That makes builds non-reproducible across the same git commit and lets a breaking upstream release silently break the lab. Bound to the current major series (community.general 9.x-10.x, mysql 4.x). Renovate (already configured for ansible-galaxy deps) will open PRs for patch bumps automatically. Co-Authored-By: Claude <noreply@anthropic.com>
Adds the standard set of files external contributors expect on a public
repo, in preparation for an OSS release:
- SECURITY.md: scopes the project's deliberately-vulnerable lab content
out of the security report channel and routes tooling vulnerabilities
through GitHub private vulnerability advisories.
- CODE_OF_CONDUCT.md: Contributor Covenant 2.1 with an added clause
clarifying that offensive-security tradecraft discussion is on-topic.
- CITATION.cff: lets researchers cite DreadGOAD in papers and reports;
references the upstream OCD/GOAD project so citations propagate.
- .github/ISSUE_TEMPLATE/{config,bug_report,feature_request}.yml:
structured issue forms with provider/lab dropdowns and a "this is
tooling, not an intentional lab vuln" gate.
- .github/PULL_REQUEST_TEMPLATE.md: provider/lab/OS prompts and a
checklist that distinguishes intentional lab credentials from real
secrets.
Co-Authored-By: Claude <noreply@anthropic.com>
A new user clones the repo and finds two entry points (the legacy Python goad.py REPL and the newer dreadgoad Go CLI) with no clear guidance on which to use when. The previous installation/usage docs still led with goad.py while README examples used dreadgoad — and dreadgoad's operational commands (provision, validate, health-check, verify-trusts, lab, ssm) are in fact AWS-only, not cross-provider as the docs implied. Adds a new docs/mkdocs/docs/cli-vs-goadpy.md migration page with the provider x tool support table, a three-line decision rule, capability matrix, and a goad.py-to-dreadgoad command equivalence table. Updates installation/index.md to drop the misleading upstream-OCD "GOAD use no more bash" paragraph and lead with the actual tool scope. Rewrites usage/index.md as three sections: AWS workflow (dreadgoad), all other providers (goad.sh), and provider-agnostic dreadgoad utilities. Rewrites providers/aws.md to drop goad.py / goad.ini / jumpbox REPL references in favor of dreadgoad infra/ami/provision/ssm flow. Adds the new page to mkdocs.yml nav and to README's Documentation section. Adds a provider-scope note to README Quick Start so it stops implying dreadgoad works on every provider. Co-Authored-By: Claude <noreply@anthropic.com>
The CLI duplicated the pattern `if cfg.Region == "" { cfg.Region = "us-west-1" }`
across 11 sites in lab.go, infra.go, infra_cmd.go, ssm.go, ami.go, and
config.go. The inventory parser had its own separate fallback to
"us-west-2" used by 5 more call sites. `dreadgoad env create` defaulted
to "us-east-1". Three different hardcoded defaults, no single source of
truth, all silent.
The user-facing failure mode was nasty: a user in eu-west-3 running
`dreadgoad lab status` against an implicit us-west-1 would just see
"no instances found" with no clue why.
This commit:
- Adds Config.ResolveRegion() (string, error) as the single source of
truth. Returns the configured region or an actionable error pointing
to all three ways to set it (dreadgoad.yaml, DREADGOAD_REGION env,
--region flag).
- Adds Config.ResolveRegionWithInventory(inv) (string, error) for code
paths that have already parsed an Ansible inventory. Prefers the
inventory's own ansible_aws_ssm_region (most authoritative — the lab
knows where it lives), falls back to ResolveRegion. Lives on Config
rather than as a free function so internal/ansible can reach it
without a new package.
- Replaces inventory.Region()'s "us-west-2" fallback with empty string,
forcing every caller to go through ResolveRegionWithInventory.
- Updates all 18 call sites to use one of the two resolvers. Special
case: ami build keeps an empty fallthrough so the warpgate template's
embedded region wins; ami list-resources / purge use ResolveRegion
because they have no template fallback.
- Fixes a UX inconsistency where ssm run required region in cfg while
ssm status / cleanup / connect would accept the inventory's region.
ssm run now parses the inventory and resolves the same way.
- Updates Config.InfraWorkDir / InfraModulePath to return (string,
error) and chain through ResolveRegion (dead code, no callers, free
to change).
- Updates --region flag help text and the config init template comment
to reflect that region is required, not "from inventory".
- Updates parser_test.go to match the new empty-fallback behavior.
Behavior change for users: AWS commands now fail fast with a clear,
actionable error if region is not configured anywhere, instead of
silently misfiring against us-west-1 / us-west-2.
Co-Authored-By: Claude <noreply@anthropic.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The previous comment claimed the warpgate template's embedded region was only overridden when the caller "explicitly provided one," but the code also overrides it whenever cfg.Region is set from dreadgoad.yaml or DREADGOAD_REGION — even though that wasn't explicit for this build. Comment now spells out the actual three-level precedence. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+130
to
138
| // Region precedence for `ami build`: --region flag > cfg.Region (from | ||
| // dreadgoad.yaml or DREADGOAD_REGION) > the template's own embedded region. | ||
| // The empty fallback (instead of a hardcoded default) is what allows the | ||
| // template's embedded region to win when neither --region nor cfg.Region | ||
| // is set; if either is set, it overrides the template even though the | ||
| // user didn't ask for this specific build. | ||
| bf := buildFlags{ | ||
| region: getFlagString(cmd, "region", cfg.Region, "us-west-1"), | ||
| region: getFlagString(cmd, "region", cfg.Region, ""), | ||
| instanceType: getFlagStringOpt(cmd, "instance-type"), |
There was a problem hiding this comment.
ami build now allows bf.region to be empty, but the build path still constructs ami.ClientConfig from bf.region (and creates the image builder) before the template is executed. If neither --region nor cfg.Region is set, this can leave the warpgate AWS client region unset and cause a runtime failure (or make the comment about "template's embedded region" inaccurate). Consider deriving the region from the loaded template (e.g., the first AMI target's region after variable interpolation) when bf.region is empty, and pass that into the AWS client config / builder initialization.
Comment on lines
+243
to
288
| // ResolveRegion returns the configured AWS region or an actionable error if | ||
| // none is set. This is the single source of truth for region resolution: every | ||
| // command that needs to talk to AWS should call it (or ResolveRegionWithInventory) | ||
| // rather than hardcoding a default. | ||
| func (c *Config) ResolveRegion() (string, error) { | ||
| if c.Region == "" { | ||
| return "", fmt.Errorf("AWS region not configured: set 'region' in dreadgoad.yaml, export DREADGOAD_REGION, or pass --region") | ||
| } | ||
| return c.Region, nil | ||
| } | ||
|
|
||
| // ResolveRegionWithInventory resolves the AWS region for talking to a deployed | ||
| // lab, preferring the parsed Ansible inventory's own region (most authoritative | ||
| // — the lab knows where it lives) and falling back to ResolveRegion. | ||
| func (c *Config) ResolveRegionWithInventory(inv *inventory.Inventory) (string, error) { | ||
| if inv != nil { | ||
| if r := inv.Region(); r != "" { | ||
| return r, nil | ||
| } | ||
| } | ||
| return c.ResolveRegion() | ||
| } | ||
|
|
||
| // InfraBasePath returns the base path for a deployment's infra directory. | ||
| func (c *Config) InfraBasePath() string { | ||
| return filepath.Join(c.ProjectRoot, "infra", c.Infra.Deployment) | ||
| } | ||
|
|
||
| // InfraWorkDir returns the working directory for terragrunt operations | ||
| // at the region level: infra/{deployment}/{env}/{region}/ | ||
| func (c *Config) InfraWorkDir() string { | ||
| region := c.Region | ||
| if region == "" { | ||
| region = "us-west-1" | ||
| func (c *Config) InfraWorkDir() (string, error) { | ||
| region, err := c.ResolveRegion() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(c.InfraBasePath(), c.Env, region) | ||
| return filepath.Join(c.InfraBasePath(), c.Env, region), nil | ||
| } | ||
|
|
||
| // InfraModulePath returns the path for a specific module within the infra working directory. | ||
| func (c *Config) InfraModulePath(module string) string { | ||
| return filepath.Join(c.InfraWorkDir(), module) | ||
| func (c *Config) InfraModulePath(module string) (string, error) { | ||
| workDir, err := c.InfraWorkDir() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return filepath.Join(workDir, module), nil | ||
| } |
There was a problem hiding this comment.
These new region-resolution APIs introduce important breaking behavior (erroring when region is unset) and new error-returning helpers (InfraWorkDir / InfraModulePath), but there are no unit tests covering the success/error cases. Adding tests for ResolveRegion, ResolveRegionWithInventory (inventory wins, nil inventory falls back), and the error paths for InfraWorkDir/InfraModulePath would help prevent regressions and validate the intended behavior.
…ion tests **Added:** - Unit tests for ResolveRegion and ResolveRegionWithInventory to verify correct region selection and error handling in different scenarios - config_test.go **Changed:** - Region flag for env create now defaults to empty and is required, with improved help text describing all ways to set the region - Logic for env creation updated to resolve region using config, environment variable, or flag, and to error clearly if no region is set - Refactored environment scaffolding logic into a new scaffoldEnv function to improve readability and separation of concerns in env_cmd.go
…ution guidelines **Changed:** - Simplified the "Deploy a Lab" section to remove provider/tool matrix and AWS-only distinctions, focusing on the main workflow for provisioning and health checks - Removed reference to the CLI vs `goad.py` comparison doc from the documentation links section in the README **Removed:** - Citation metadata file for academic citation information (CITATION.cff) - Contributor Covenant Code of Conduct and enforcement guidelines (CODE_OF_CONDUCT.md) - Security policy outlining vulnerability reporting and project scope (SECURITY.md) - Detailed documentation comparing the Go CLI (`dreadgoad`) and Python REPL (`goad.sh`) across providers, including usage tables and workflows (docs/mkdocs/docs/cli-vs-goadpy.md)
**Removed:** - Removed "🧭 CLI vs goad.py" entry from the documentation navigation in mkdocs.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
OSS readiness pass
Prep work for opening DreadGOAD as a public OSS project: standard hygiene files, doc clarification, dependency pins, and a region-resolution refactor.
Added
SECURITY.md,CODE_OF_CONDUCT.md,CITATION.cff.github/ISSUE_TEMPLATE/(bug + feature + contact links) andPULL_REQUEST_TEMPLATE.mddocs/mkdocs/docs/cli-vs-goadpy.md— provider × tool support matrix and command equivalence betweendreadgoadandgoad.pyConfig.ResolveRegion()/Config.ResolveRegionWithInventory()as the single source of truth for AWS region resolutionChanged
dreadgoad(AWS only for operational commands) vsgoad.sh(VirtualBox / VMware / Proxmox /Azure / Ludus)community.generalandgeerlingguy.mysqlare now version-pinned inansible/galaxy.ymlandansible/requirements.yml; Renovate will track them automatically--regionflag anddreadgoad config showhelp text now reflect that region is required for AWS commandsBreaking
us-west-1/us-west-2. Set it viaregion:indreadgoad.yaml,DREADGOAD_REGION, or--region— the error message points to all three.inventory.Inventory.Region()returns""instead of"us-west-2"when the inventory has noansible_aws_ssm_region. Out-of-tree code calling this directly should switch toConfig.ResolveRegionWithInventory(inv).Config.InfraWorkDir()andConfig.InfraModulePath()now return(string, error).Fixed
dreadgoad ssm runis now consistent with the rest of thessmsubcommands and accepts the inventory's region as a fallbackdreadgoad provisionnow fails fast with an actionable region error instead of hitting a confusing SSM connection error several minutes into the playbook runNotes
cli/go.modstill pinsgithub.com/cowdogmoo/warpgate/v3to a pseudo-version. Either tag a stable release upstream or document the snapshot before merging.cd cli && go build ./... && go test ./...before merging — this branch was reviewed and edited without a local Go toolchain.