mirror of
https://github.com/Sea-Haven-Industries/engineering-handbook.git
synced 2026-09-30 08:03:16 +00:00
Merge pull request #9 from Sea-Haven-Industries/feature/jira-linking-convention
Add Jira issue-linking convention + handbook index/standards cleanup
This commit is contained in:
Adam Moussa
GitHub
commit
8ebf52b5e6
6 changed files with 298 additions and 18 deletions
|
|
@ -10,9 +10,11 @@ Engineering conventions and best practices for Sea Haven Industries.
|
|||
- [Commit Messages](commit-messages.md) -- imperative mood, 50/72 rule, explain "why"
|
||||
- [Pull Requests](pull-requests.md) -- scope, title, description format, merge strategy
|
||||
- [Code Review](code-review.md) -- what to look for, giving feedback, turnaround expectations
|
||||
- [Code Review Rubric](code-review-rubric.md) -- BLOCK/FIX/NIT/QUESTION finding categories and output format
|
||||
- [GitHub Standards](github-standards.md) -- branch defaults, repo hygiene, Dependabot
|
||||
- [AWS Infrastructure](aws-infrastructure.md) -- SAM vs CDK, Lambda defaults, CloudFormation
|
||||
- [SAM Project Layout](sam-project-layout.md) -- standard directory structure for serverless projects
|
||||
- [CDK Project Layout](cdk-project-layout.md) -- standard directory structure for CDK projects
|
||||
- [Lambda Starter Template](lambda-template.md) -- minimal SAM scaffold for a new Python Lambda
|
||||
- [Secrets and Configuration](secrets-and-config.md) -- Secrets Manager vs SSM Parameter Store
|
||||
- [CI/CD Pipelines](cicd.md) -- every deployable repo gets a pipeline, no manual deploys
|
||||
|
|
|
|||
219
cdk-project-layout.md
Normal file
219
cdk-project-layout.md
Normal file
|
|
@ -0,0 +1,219 @@
|
|||
# CDK Project Layout
|
||||
|
||||
## When to Use CDK
|
||||
|
||||
SAM is the default for new serverless stacks (Lambda + API Gateway + DynamoDB). Use CDK when the infrastructure goes beyond what SAM handles cleanly:
|
||||
|
||||
- ECS Fargate tasks
|
||||
- VPCs, subnets, security groups
|
||||
- Multi-service compositions (e.g., SES + Lambda + DynamoDB + Bedrock Agent)
|
||||
- L2/L3 constructs not available in SAM (Bedrock agents, Knowledge Bases)
|
||||
- Docker-based Lambda or container workloads
|
||||
|
||||
If the project is a handful of Lambdas behind API Gateway, use SAM. See [sam-project-layout.md](sam-project-layout.md).
|
||||
|
||||
## Standard Directory Structure
|
||||
|
||||
```
|
||||
project-name/
|
||||
├── bin/
|
||||
│ └── app.ts # CDK app entry point
|
||||
├── lib/
|
||||
│ └── project-name-stack.ts # Stack definition
|
||||
├── lambdas/
|
||||
│ ├── function-name/
|
||||
│ │ ├── index.ts # Handler (Node) or app.py (Python)
|
||||
│ │ └── package.json # Per-function deps (Node) or requirements.txt (Python)
|
||||
│ └── shared/ # Shared utilities across functions (if needed)
|
||||
├── cdk.json
|
||||
├── package.json
|
||||
├── package-lock.json
|
||||
├── tsconfig.json
|
||||
├── .gitignore
|
||||
└── .github/
|
||||
└── workflows/
|
||||
├── ci.yaml
|
||||
└── deploy.yaml
|
||||
```
|
||||
|
||||
## File Purposes
|
||||
|
||||
### `bin/app.ts`
|
||||
|
||||
The CDK app entry point. Instantiates the stack with an explicit `stackName` to enforce kebab-case (see Naming below).
|
||||
|
||||
```typescript
|
||||
import * as cdk from 'aws-cdk-lib';
|
||||
import { ProjectNameStack } from '../lib/project-name-stack';
|
||||
|
||||
const app = new cdk.App();
|
||||
new ProjectNameStack(app, 'ProjectNameStack', {
|
||||
stackName: 'project-name',
|
||||
env: { account: '328440206208', region: 'us-east-1' },
|
||||
});
|
||||
```
|
||||
|
||||
### `lib/project-name-stack.ts`
|
||||
|
||||
All resource definitions. For larger projects, split into multiple constructs under `lib/` and compose them in the stack file. Keep the stack class thin -- it wires constructs together, not defines low-level resources.
|
||||
|
||||
### `lambdas/`
|
||||
|
||||
Lambda handler source code. One directory per function, same convention as SAM's `src/` directory. Each function has its own dependency file. CDK references these via `Code.fromAsset('lambdas/function-name')` or `NodejsFunction`'s `entry` property.
|
||||
|
||||
### `cdk.json`
|
||||
|
||||
CDK context and feature flags. Committed to the repo. Generated by `cdk init` -- keep the default `app` command pointing at `bin/app.ts`.
|
||||
|
||||
## Naming
|
||||
|
||||
CDK generates PascalCase stack names by default. Always set an explicit `stackName` to enforce kebab-case:
|
||||
|
||||
```typescript
|
||||
new MyStack(app, 'MyStack', {
|
||||
stackName: 'my-stack',
|
||||
});
|
||||
```
|
||||
|
||||
All resource names within the stack follow the same kebab-case convention: `my-stack-process-orders` for Lambdas, `my-stack-orders` for tables, `my-stack/api-key` for secrets. See [naming-conventions.md](naming-conventions.md).
|
||||
|
||||
### Legacy stacks
|
||||
|
||||
A handful of stacks predate this convention and remain PascalCase (e.g., `SeaHavenDoorUnlockStack`). Do not rename these -- changing the stack name requires replacement of all resources. New stacks must use kebab-case from day one.
|
||||
|
||||
## Version Pinning
|
||||
|
||||
Pin `aws-cdk-lib` to the blessed version: **2.253.1**.
|
||||
|
||||
`aws-cdk-lib` bundles transitive dependencies (`inBundle: true`). Certain versions break `npm ci` with phantom missing-package errors that cannot be fixed via npm `overrides`. Always test before bumping:
|
||||
|
||||
1. Update `package.json` to the new version
|
||||
2. Run `rm -rf node_modules package-lock.json && npm install`
|
||||
3. Run `npm ci` -- if it fails, the version is not safe
|
||||
4. Run `npx cdk synth` -- if it fails, the version is not safe
|
||||
|
||||
Dependabot will flag vulnerabilities in bundled transitive deps. Dismiss these alerts with **"waiting for upstream fix"** -- there is no action available until `aws-cdk-lib` publishes a patched release.
|
||||
|
||||
See [aws-infrastructure.md](aws-infrastructure.md#cdk-version-policy) for more detail.
|
||||
|
||||
## overrideLogicalId
|
||||
|
||||
Never remove `overrideLogicalId` calls from existing resources.
|
||||
|
||||
CDK auto-generates CloudFormation logical IDs with hash suffixes (e.g., `OrdersTable4A3B2C1D`). When you override the logical ID to something stable (e.g., `OrdersTable`), CloudFormation tracks the resource under that name. Removing the override changes the logical ID back to the hashed version, which CloudFormation interprets as:
|
||||
|
||||
1. **Delete** the resource with the old logical ID
|
||||
2. **Create** a new resource with the new logical ID
|
||||
|
||||
For named resources (Secrets Manager secrets, S3 buckets, IAM roles), the create fails because the physical name already exists. For unnamed resources (DynamoDB tables without explicit names), the delete succeeds -- and takes your data with it.
|
||||
|
||||
If you need to refactor construct tree paths, add new `overrideLogicalId` calls to preserve the existing logical IDs. Never remove existing ones without understanding the CloudFormation diff (`cdk diff`).
|
||||
|
||||
## ARM64 Docker Builds
|
||||
|
||||
When using `ContainerImage.fromAsset()` for ARM64 Fargate tasks (or any ARM64 container), two things are required:
|
||||
|
||||
### 1. Platform flag in CDK
|
||||
|
||||
```typescript
|
||||
import * as ecr_assets from 'aws-cdk-lib/aws-ecr-assets';
|
||||
|
||||
ContainerImage.fromAsset('path/to/docker', {
|
||||
platform: ecr_assets.Platform.LINUX_ARM64,
|
||||
});
|
||||
```
|
||||
|
||||
Without the `platform` flag, CDK builds an x86 image regardless of what the Dockerfile or CI runner does. The container will crash on an ARM64 Fargate task with `exec format error`.
|
||||
|
||||
### 2. QEMU in GitHub Actions
|
||||
|
||||
The CI runner is x86. To build ARM64 images, the workflow needs QEMU:
|
||||
|
||||
```yaml
|
||||
- uses: docker/setup-qemu-action@v3
|
||||
```
|
||||
|
||||
Add this step before `cdk deploy` or `cdk synth` in the deploy workflow. Both the platform flag and QEMU are required -- one without the other produces a broken image.
|
||||
|
||||
## Lambda Defaults
|
||||
|
||||
Same defaults as SAM projects. Verify these on every Lambda in every CDK stack:
|
||||
|
||||
| Setting | Value |
|
||||
|---|---|
|
||||
| Runtime | Python 3.12 or Node 24.x |
|
||||
| Architecture | arm64 |
|
||||
| Log retention | 60 days (explicit `RetentionInDays`) |
|
||||
| Naming | kebab-case, prefixed with stack name |
|
||||
|
||||
CDK's `NodejsFunction` and `PythonFunction` constructs auto-create log groups, but the default retention is **never expire**. Always set it explicitly:
|
||||
|
||||
```typescript
|
||||
new logs.LogGroup(this, 'HandlerLogs', {
|
||||
logGroupName: `/aws/lambda/${fn.functionName}`,
|
||||
retention: logs.RetentionDays.TWO_MONTHS,
|
||||
});
|
||||
```
|
||||
|
||||
See [aws-infrastructure.md](aws-infrastructure.md#lambda-defaults).
|
||||
|
||||
## CI/CD
|
||||
|
||||
Same OIDC deploy role pattern as SAM. Each repo gets its own `githubdeploy-<repo-name>` IAM role -- never share deploy roles across repos.
|
||||
|
||||
CDK projects use the TypeScript CDK reusable workflows:
|
||||
|
||||
```yaml
|
||||
# .github/workflows/ci.yaml
|
||||
name: CI
|
||||
on:
|
||||
pull_request:
|
||||
branches: [main]
|
||||
jobs:
|
||||
ci:
|
||||
uses: Sea-Haven-Industries/.github/.github/workflows/ci-typescript-cdk.yaml@main
|
||||
with:
|
||||
node-version: "24"
|
||||
|
||||
# .github/workflows/deploy.yaml
|
||||
name: Deploy
|
||||
on:
|
||||
push:
|
||||
branches: [main]
|
||||
jobs:
|
||||
deploy:
|
||||
uses: Sea-Haven-Industries/.github/.github/workflows/cd-cdk.yaml@main
|
||||
with:
|
||||
node-version: "24"
|
||||
secrets:
|
||||
deploy-role-arn: ${{ secrets.AWS_DEPLOY_ROLE_ARN }}
|
||||
```
|
||||
|
||||
Always pass `node-version: "24"` explicitly. See [cicd.md](cicd.md) for the full pipeline convention.
|
||||
|
||||
## Bedrock Agents
|
||||
|
||||
CDK is the standard IaC for Bedrock Agents and Knowledge Bases (SAM lacks L2 constructs for these). Key gotchas:
|
||||
|
||||
- Use cross-region inference profile IDs for `CfnAgent.foundationModel`, not direct model IDs
|
||||
- Bump the alias `description` to force version rotation after model or instruction changes
|
||||
- `VectorKnowledgeBase` requires Docker Desktop on the build machine
|
||||
|
||||
See [bedrock.md](bedrock.md) for inference profile formats, IAM permissions, alias version pinning, and action group conventions.
|
||||
|
||||
## Standard .gitignore
|
||||
|
||||
```
|
||||
cdk.out/
|
||||
node_modules/
|
||||
*.js
|
||||
*.d.ts
|
||||
*.js.map
|
||||
.env
|
||||
```
|
||||
|
||||
The `*.js` / `*.d.ts` / `*.js.map` entries assume TypeScript source with compiled output excluded from version control. If you have JavaScript Lambda handlers under `lambdas/`, add a negation:
|
||||
|
||||
```
|
||||
!lambdas/**/*.js
|
||||
```
|
||||
36
code-review-rubric.md
Normal file
36
code-review-rubric.md
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
# Code Review Rubric
|
||||
|
||||
## Finding Categories
|
||||
|
||||
| Category | Meaning | Action Required |
|
||||
|---|---|---|
|
||||
| BLOCK | Must fix before merge | Yes -- PR cannot merge |
|
||||
| FIX | Should fix, strong recommendation | Yes -- unless explicitly deferred with issue |
|
||||
| NIT | Optional improvement, style preference | No -- author's discretion |
|
||||
| QUESTION | Needs clarification before reviewer can assess | Yes -- answer required |
|
||||
|
||||
## Review Checklist
|
||||
|
||||
- Correctness: does the code do what the PR says?
|
||||
- Security: OWASP top 10, secrets handling, input validation at boundaries
|
||||
- Sea Haven conventions: naming, secrets placement, Lambda defaults, IaC patterns
|
||||
- Operational readiness: logging, error handling, monitoring, CI/CD
|
||||
- Tests: appropriate coverage for the change
|
||||
|
||||
## Output Format
|
||||
|
||||
```
|
||||
Verdict: APPROVE | REQUEST CHANGES | NEEDS DISCUSSION
|
||||
|
||||
### BLOCK
|
||||
- **file.py:42** -- [problem]. Why it matters: [impact]. Fix: [suggestion].
|
||||
|
||||
### FIX
|
||||
- ...
|
||||
|
||||
### NIT
|
||||
- ...
|
||||
|
||||
### QUESTION
|
||||
- ...
|
||||
```
|
||||
|
|
@ -66,9 +66,18 @@ Optional body wrapped at 72 characters. Explain the problem
|
|||
this commit solves and why this approach was chosen. Mention
|
||||
side effects or non-obvious consequences.
|
||||
|
||||
Refs: #123
|
||||
Refs: PROJ-123, #123
|
||||
```
|
||||
|
||||
## Referencing issues
|
||||
|
||||
Use the `Refs:` trailer to point at the work this commit relates to:
|
||||
|
||||
- **Jira key** (`PROJ-123`) when the work tracks a Jira issue. The GitHub for Jira app reads the key and threads the commit into the issue's development panel. See [git-workflow.md](git-workflow.md#linking-to-jira).
|
||||
- **GitHub issue** (`#123`) when the work tracks a GitHub issue in the same repo.
|
||||
|
||||
List both when both apply: `Refs: PROJ-123, #123`. The key only needs to appear once in the branch, PR title, or any commit for the link to form, but including it in the trailer keeps the reference attached to the individual change.
|
||||
|
||||
## Example
|
||||
|
||||
```
|
||||
|
|
@ -79,5 +88,5 @@ deployments. Without retries, these surface as user-facing
|
|||
errors. This adds exponential backoff with 3 attempts,
|
||||
which matches the upstream's documented recovery window.
|
||||
|
||||
Refs: #45
|
||||
Refs: PROJ-123
|
||||
```
|
||||
|
|
|
|||
|
|
@ -8,18 +8,44 @@
|
|||
|
||||
| Prefix | Use when |
|
||||
|---|---|
|
||||
| `feature/<description>` | Adding new functionality or enhancing existing features |
|
||||
| `bug/<description>` | Fixing a non-urgent defect found during development or testing |
|
||||
| `hotfix/<description>` | Fixing a production issue that needs immediate attention |
|
||||
| `feature/<key>-<description>` | Adding new functionality or enhancing existing features |
|
||||
| `bug/<key>-<description>` | Fixing a non-urgent defect found during development or testing |
|
||||
| `hotfix/<key>-<description>` | Fixing a production issue that needs immediate attention |
|
||||
|
||||
**bug vs hotfix:** Use `bug/` for defects caught before they affect production (failing tests, broken dev flows, issues found in review). Use `hotfix/` only when production is impacted and the fix needs to bypass normal review cadence.
|
||||
|
||||
**Jira key:** When the work tracks a Jira issue, put the key after the prefix: `feature/PROJ-123-add-receipt-parser` (substitute the real project key, e.g. the infra or software-development project). This is what wires the branch, commits, and PR into the issue's development panel. See [Linking to Jira](#linking-to-jira) below. Work with no Jira issue (one-off scripts, trivial fixes) omits the key and uses a plain description.
|
||||
|
||||
## Commits
|
||||
|
||||
- Commit each logical change individually with a descriptive message
|
||||
- See [commit-messages.md](commit-messages.md) for formatting rules
|
||||
- Keep the working tree clean: commit or stash before switching context
|
||||
|
||||
## Linking to Jira
|
||||
|
||||
Work is tracked in Jira (`seahaven.atlassian.net`). Jira is the source of truth for *work*; GitHub is the source of truth for *code*. We do not duplicate issues between the two — we link them.
|
||||
|
||||
The org-level **GitHub for Jira** app is already installed across all repos. It detects the Jira issue key (e.g. `PROJ-123`) wherever it appears and surfaces the branch, commits, PR, and deployment status in that issue's development panel automatically. To make that happen, mention the key in at least one of:
|
||||
|
||||
- the branch name (`feature/PROJ-123-add-receipt-parser`)
|
||||
- the PR title (`[PROJ-123] Add receipt parser`)
|
||||
- a commit message (the `Refs:` trailer, see [commit-messages.md](commit-messages.md))
|
||||
|
||||
Mentioning it in the branch name covers all three at once, so that is the minimum bar.
|
||||
|
||||
### Smart Commit commands
|
||||
|
||||
The integration also accepts inline commands in commit messages to act on the issue without opening Jira:
|
||||
|
||||
| Command | Effect |
|
||||
|---|---|
|
||||
| `PROJ-123 #comment <text>` | Add a comment to the issue |
|
||||
| `PROJ-123 #time 2h <text>` | Log work |
|
||||
| `PROJ-123 #done` (or another transition name) | Transition the issue |
|
||||
|
||||
Transition names are case-insensitive and match the issue's workflow (`#in-progress`, `#done`). Use these sparingly; the link itself is the main goal, not driving the whole workflow from commits.
|
||||
|
||||
## Deploy-Then-Merge
|
||||
|
||||
The standard flow for changes that deploy to AWS:
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@
|
|||
|
||||
## Dependabot Configuration
|
||||
|
||||
Every active repo with package dependencies must have a `.github/dependabot.yml` that covers all relevant ecosystems. All entries must assign PRs to `amoussa1229`.
|
||||
Every active repo with package dependencies must have a `.github/dependabot.yml` that covers all relevant ecosystems.
|
||||
|
||||
### Ecosystem Selection
|
||||
|
||||
|
|
@ -35,8 +35,6 @@ updates:
|
|||
directory: "/"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
```
|
||||
|
||||
**SAM project with per-function `requirements.txt`:**
|
||||
|
|
@ -50,14 +48,10 @@ updates:
|
|||
directory: "/src/processor"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
- package-ecosystem: "pip"
|
||||
directory: "/src/receiver"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
```
|
||||
|
||||
**Mixed ecosystems (e.g., CDK in JS with Python Lambdas, or repos with GitHub Actions):**
|
||||
|
|
@ -71,20 +65,14 @@ updates:
|
|||
directory: "/"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
- package-ecosystem: "pip"
|
||||
directory: "/src"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
- package-ecosystem: "github-actions"
|
||||
directory: "/"
|
||||
schedule:
|
||||
interval: "weekly"
|
||||
assignees:
|
||||
- "amoussa1229"
|
||||
```
|
||||
|
||||
### Merging Dependabot PRs
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue