1.7.3 #196
1.7.3 #196
Conversation
WalkthroughVersion bumped to v1.7.3 across release artifacts; Go toolchain updated to 1.24.6. Added AWS/Dagster systemd startup scripts and RDS health checks, expanded infrastructure docs, and applied CLI validation and small refactors (API URL, secret key). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant systemd as systemd
participant wait as wait-for-rds.sh
participant secrets as start-dagster.sh
participant aws as AWS Secrets Manager
participant dagster as Dagster (dg)
rect rgb(240,248,255)
Note over systemd,wait: service start → ensure DB reachable
systemd ->> wait: ExecStartPre /home/ubuntu/wait-for-rds.sh
wait ->> aws: (uses $AWS_RDS_HOSTNAME) nc poll until reachable
wait -->> systemd: exit 0 (ready) / exit 1 (timeout)
end
rect rgb(245,255,245)
Note over systemd,secrets: fetch secrets and start Dagster
systemd ->> secrets: ExecStart /home/ubuntu/start-dagster.sh
secrets ->> aws: GetSecretValue (supabase, rds)
aws -->> secrets: SecretString JSON
secrets ->> secrets: parse with jq, export env vars, activate venv
secrets ->> dagster: exec dg dev --host 0.0.0.0 --port 3000
dagster -->> systemd: stdout/stderr -> journal
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25–30 minutes
Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is
... and 20 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
docs/Infrastructure_Guide/index.md (1)
5-5: Verify header format aligns with documentation style guide.The header has been updated from
# 1. Overviewto# 1 // Overview. While this is a minor change, please confirm that this format is intentional and consistent with other section headers across the documentation.If this is part of a broader documentation style guide update, consider verifying consistency across related documentation files (e.g., other
index.mdfiles in the documentation structure, or similar numbered section headers).docs/Infrastructure_Guide/supabase.md (2)
5-5: Consider removing the numeric prefix from the title.The title
# 2 // Supabaseincludes a numeric prefix that duplicates the ordering already specified in the frontmatterweight: 2. This makes the title brittle—if the weight changes, the title becomes inconsistent. Titles should be human-readable labels, not structural indices.Apply this diff to decouple the title from section numbering:
-# 2 // Supabase +# SupabaseThis relies on the frontmatter weight to control ordering and keeps the title clean and maintainable.
12-33: Well-structured procedural section with clear instructions.The new "Retrieve PostgreSQL Connection String" section is comprehensive, actionable, and properly cross-referenced. The step-by-step guidance with nested sub-steps is easy to follow, and the example connection string format is helpful.
Minor suggestion: Line 27 could explicitly note that placeholders in the connection string must be replaced with actual values:
-5. The connection string will be provided in the following format: +5. The connection string will be provided in the following format (replace placeholders with your actual values):This is optional and won't block merge.
flags/pokemonflagset.go (1)
138-138: Consider simplifying the API function signatures.Since all callers now consistently pass
connections.APIURL, the API functions (PokemonApiCall,TypesApiCall,MoveApiCall) could potentially useAPIURLdirectly rather than requiring it as a parameter. This would reduce repetition across all call sites.However, keeping the parameter does provide flexibility for testing and potential future use cases with different endpoints.
Also applies to: 152-152, 322-322, 411-411, 446-446, 556-556, 651-651
cmd/utils/validateargs.go (1)
45-55: LGTM! Consistent validation logic.The implementation correctly validates berry command arguments and follows the same pattern as other command validators.
Optional: Consider reducing duplication across validators.
ValidateBerryArgs, ValidateNaturesArgs, ValidateSearchArgs, ValidateSpeedArgs, and ValidateTypesArgs share nearly identical implementations (all call checkLength with max 3 and checkNoOtherOptions with max 3, differing only in commandName). You could introduce a helper function to reduce this duplication:
func validateSimpleCommand(args []string, commandName string) error { if err := checkLength(args, 3); err != nil { return err } if err := checkNoOtherOptions(args, 3, commandName); err != nil { return err } return nil } func ValidateBerryArgs(args []string) error { return validateSimpleCommand(args, "<berry>") } func ValidateNaturesArgs(args []string) error { return validateSimpleCommand(args, "<natures>") } // ... and so on for other similar validatorscard_data/infrastructure/wait-for-rds.sh (1)
12-12: Add robustness checks for external tool dependency.Line 12 uses
nc(netcat), which may not be available on all systems. Consider adding an early check or documenting this as a required dependency:# At the top of the script: if ! command -v nc &> /dev/null; then echo "Error: 'nc' (netcat) is not installed" exit 1 fidocs/Infrastructure_Guide/aws.md (1)
308-325: Consider emphasizing placeholder values in the example.The example AWS Secrets Manager command includes credential placeholders that users must replace. While the placeholders are present, it might be helpful to add a note emphasizing that users should never commit real credentials to documentation or code.
Consider adding a warning note before the command:
Create a secret for Supabase credentials[^1]: + +!!! warning "Replace Placeholder Values" + + Ensure you replace `supabase-password` and `supabase-user` with your actual + credentials. Never commit real credentials to version control. ```shellcard_data/infrastructure/start-dagster.sh (1)
38-42: Verify virtual environment exists before activation.The script sources the virtual environment without checking if it exists. If the path is incorrect or the venv wasn't created, the script will fail with a cryptic error.
Apply this diff:
# Activate the virtual environment +VENV_PATH="/home/ubuntu/card_data/card_data/.venv/bin/activate" +if [ ! -f "$VENV_PATH" ]; then + echo "Error: Virtual environment not found at $VENV_PATH" >&2 + exit 1 +fi -source /home/ubuntu/card_data/card_data/.venv/bin/activate +source "$VENV_PATH" # Start Dagster exec dg dev --host 0.0.0.0 --port 3000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (29)
.github/workflows/ci.yml(2 hunks).github/workflows/go_test.yml(1 hunks).goreleaser.yml(1 hunks)Dockerfile(2 hunks)README.md(2 hunks)card_data/dagster.yaml(1 hunks)card_data/infrastructure/dagster.service(1 hunks)card_data/infrastructure/start-dagster.sh(1 hunks)card_data/infrastructure/wait-for-rds.sh(1 hunks)card_data/pipelines/poke_cli_dbt/dbt_project.yml(1 hunks)card_data/pipelines/utils/secret_retriever.py(1 hunks)cmd/berry/berry.go(1 hunks)cmd/search/search.go(1 hunks)cmd/utils/validateargs.go(2 hunks)docs/Infrastructure_Guide/aws.md(4 hunks)docs/Infrastructure_Guide/cloud-deployment.md(1 hunks)docs/Infrastructure_Guide/dagster.md(0 hunks)docs/Infrastructure_Guide/dbt.md(0 hunks)docs/Infrastructure_Guide/index.md(1 hunks)docs/Infrastructure_Guide/local-deployment.md(1 hunks)docs/Infrastructure_Guide/python.md(0 hunks)docs/Infrastructure_Guide/supabase.md(1 hunks)docs/Infrastructure_Guide/terraform.md(2 hunks)docs/stylesheets/extra.css(1 hunks)flags/pokemonflagset.go(7 hunks)go.mod(3 hunks)mkdocs.yml(3 hunks)nfpm.yaml(1 hunks)testdata/main_latest_flag.golden(1 hunks)
💤 Files with no reviewable changes (3)
- docs/Infrastructure_Guide/dagster.md
- docs/Infrastructure_Guide/python.md
- docs/Infrastructure_Guide/dbt.md
🧰 Additional context used
🧬 Code graph analysis (2)
cmd/berry/berry.go (1)
cmd/utils/validateargs.go (1)
ValidateBerryArgs(45-55)
flags/pokemonflagset.go (1)
connections/connection.go (4)
PokemonApiCall(112-127)APIURL(17-17)TypesApiCall(148-160)MoveApiCall(94-109)
🪛 LanguageTool
docs/Infrastructure_Guide/local-deployment.md
[grammar] ~60-~60: Use a hyphen to join words.
Context: ...base. The public schema is the public facing schema that exposes the API to th...
(QB_NEW_EN_HYPHEN)
[style] ~91-~91: Using many exclamation marks might seem excessive (in this case: 6 exclamation marks for a text that’s 1845 characters long)
Context: ...the staging schema. --- ## Dagster !!! question "What is Dagster?" Dagste...
(EN_EXCESSIVE_EXCLAMATION)
docs/Infrastructure_Guide/cloud-deployment.md
[style] ~114-~114: Consider a more concise word here.
Context: ...ting Startup with systemd Optional In order to save on costs, the EC2 and RDS instance...
(IN_ORDER_TO_PREMIUM)
[grammar] ~116-~116: Ensure spelling is correct
Context: ...To automate the starting of the Dagster webservice, systemd, along with a couple of she...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~157-~157: Use a hyphen to join words.
Context: ... Copy or move the files from the checked out repository to the proper directory o...
(QB_NEW_EN_HYPHEN)
[grammar] ~158-~158: Use a hyphen to join words.
Context: ...s must first be edited to match project specific configuration. Such as the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~169-~169: Use a hyphen to join words.
Context: ...ted. Update the files below with project specific configuration then run the `cat...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/Infrastructure_Guide/local-deployment.md
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
130-130: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
docs/Infrastructure_Guide/cloud-deployment.md
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
130-130: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
152-152: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
178-178: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
222-222: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
260-260: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: tests
🔇 Additional comments (24)
flags/pokemonflagset.go (1)
138-138: LGTM! Consistent API URL usage across all flag functions.The migration from hard-coded
baseURLtoconnections.APIURLimproves consistency and maintainability. All flag functions now follow the same pattern asAbilitiesFlag(line 85), centralizing the API endpoint configuration.Also applies to: 152-152, 322-322, 411-411, 446-446, 556-556, 651-651
cmd/utils/validateargs.go (1)
6-7: LGTM!The import reordering is a styling change with no functional impact.
cmd/berry/berry.go (1)
42-42: LGTM! Correct validator now used.The berry command now uses the appropriate
ValidateBerryArgsvalidator instead of the incorrectValidateTypesArgs. This properly fixes the argument validation for the berry command.cmd/search/search.go (1)
40-42: LGTM! Improved error handling consistency.Adding
os.Exit(1)ensures the program terminates with a non-zero exit status when startup fails, which is consistent with the validation error handling at line 36. This properly signals failure to calling processes..goreleaser.yml (1)
17-17: LGTM!Version bump to v1.7.3 is correctly applied in the ldflags configuration.
card_data/pipelines/poke_cli_dbt/dbt_project.yml (3)
2-2: LGTM!Version bump to 1.7.3 is correctly applied and consistent with the PR-wide version update.
21-21: LGTM!Adding the
materialized: tableconfiguration explicitly sets the materialization strategy for the poke_cli_dbt models.
23-24: Clarify: on-run-end hook appears unchanged.The AI summary states the on-run-end hook line was removed, but lines 23-24 show the hook is still present (these lines are not marked as changed with
~). If the hook was meant to be removed, it should be deleted. If it's intentionally kept, the summary is inconsistent with the actual changes.Dockerfile (2)
2-2: LGTM!Go toolchain update to 1.24.6 aligns with the workflow files (.github/workflows/go_test.yml line 19) and go.mod updates.
11-11: LGTM!Version string update to v1.7.3 is consistent with the PR-wide version bump.
nfpm.yaml (1)
4-4: LGTM!Version bump to v1.7.3 is correctly applied in the nfpm package configuration, consistent with updates in .goreleaser.yml, Dockerfile, and other artifacts.
card_data/dagster.yaml (1)
1-12: Environment variables confirmed to be properly set by infrastructure setup.The Postgres storage configuration correctly uses environment variables (
AWS_RDS_PASSWORD,AWS_RDS_HOSTNAME) for sensitive credentials rather than hardcoding them. The SSL requirement (sslmode: require) is appropriate for AWS RDS connections. Verification confirms that both variables are properly extracted and exported incard_data/infrastructure/start-dagster.sh(lines 29-30 and 32-33) before Dagster starts..github/workflows/go_test.yml (1)
19-19: Go 1.24.6 is confirmed as publicly available.Go 1.24.6 was released on August 6, 2025, and is available for download from the official Go downloads page. The workflow update is valid.
go.mod (2)
16-16: Verify modernc.org/sqlite promotion from indirect to direct dependency.Line 16 shows
modernc.org/sqlite v1.39.1promoted to a direct dependency (previously indirect). Please confirm this change is intentional and clarify which new code or functionality requires this SQLite package as a direct import.
3-3: Version bumps appear reasonable.Go toolchain (1.24.5 → 1.24.6), golang.org/x/sys, and modernc.org/libc updates are minor patches. Ensure the team has tested these against the current codebase and there are no compatibility regressions.
Also applies to: 48-48, 50-50
docs/Infrastructure_Guide/local-deployment.md (1)
1-5: Comprehensive local deployment guide added.This is a well-structured new documentation file covering Python (uv), dbt, and Dagster setup with clear examples and project layout. The content is accurate and helpful for users setting up locally.
docs/Infrastructure_Guide/cloud-deployment.md (2)
86-100: LGTM!The
dagster.yamlconfiguration template is well-structured with clear placeholders for sensitive information.
112-332: Excellent systemd automation documentation.The systemd setup guide is comprehensive and includes:
- Clear prerequisites for required tools (
netcat,jq)- Both file copy and manual creation options
- Appropriate security configurations
- Helpful commands for service management
The placeholder at line 272 (
<correct-rds-secret>) is clearly marked for user customization.docs/Infrastructure_Guide/aws.md (2)
198-209: LGTM!The Elastic IPs section provides clear, step-by-step instructions for allocating and associating a static IP address to an EC2 instance.
212-297: Excellent EventBridge automation guide.The EventBridge section provides comprehensive coverage of:
- IAM policy and role creation
- Schedule configuration with cron expressions
- Step-by-step wizard navigation
This is a valuable addition for cost optimization through automated instance scheduling.
card_data/infrastructure/start-dagster.sh (1)
1-2: LGTM!The shebang is correct for a bash script.
card_data/infrastructure/dagster.service (3)
1-6: LGTM!The Unit section properly configures network dependencies with
After=network-online.targetandWants=network-online.target, ensuring the service starts only after network connectivity is established.
7-22: Well-configured service with security hardening.The Service section implements several security best practices:
NoNewPrivileges=true: Prevents privilege escalationPrivateTmp=true: Provides isolated tmp directoryProtectSystem=strict: Makes most of the filesystem read-onlyProtectHome=read-only: Allows reading from home but prevents writesRestart=on-failure: Automatic recovery from failuresThe
ExecStartPredirective properly sequences RDS availability checks before starting Dagster.
24-25: LGTM!The Install section correctly targets
multi-user.target, which is appropriate for a service that should start at boot in multi-user mode.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
docs/Infrastructure_Guide/cloud-deployment.md (1)
29-35: Fix region inconsistency and bashrc path typo (unresolved past comment).Lines 31 and 33 specify different AWS regions (us-west-2 vs us-west-1), which will confuse users. Additionally, line 32 contains a typo:
~./bashrcshould be~/.bashrc.Apply this diff to make the documentation consistent:
* Set the default region: ```shell export AWS_DEFAULT_REGION=us-west-2 - # or set it to ~./bashrc file - echo 'export AWS_DEFAULT_REGION=us-west-1' >> ~/.bashrc + # or set it to ~/.bashrc file + echo 'export AWS_DEFAULT_REGION=us-west-2' >> ~/.bashrc source ~/.bashrc ```
🧹 Nitpick comments (3)
docs/Infrastructure_Guide/cloud-deployment.md (3)
112-117: Improve markdown structure and wording clarity.Line 113 uses emphasis (
_Optional_) rather than a structured markdown element (e.g., note block or admonition). Line 114 uses verbose phrasing ("In order to save on costs") that could be concise. Line 116 may need spacing adjustment for "web service" or "web-service".Consider using an admonition or note block for optional status, and tighten the phrasing:
## Automating Startup with `systemd` -_Optional_ -In order to save on costs, the EC2 and RDS instances are scheduled to start and -stop once each day with AWS EventBridge. To automate the starting of the Dagster webservice, -`systemd`, along with a couple of shell scripts, will be used to create this automation. +!!! note "Optional" + To reduce costs, EC2 and RDS instances are scheduled to start and stop daily with AWS EventBridge. + This section shows how to automate Dagster startup using `systemd` and shell scripts.
130-130: Apply hyphenation and heading level corrections.Multiple minor grammar and markdown formatting issues:
- Line 130: Heading level should be h4 (
####) not h5 (#####) to maintain proper hierarchy- Lines 157, 158, 169: Compound adjectives require hyphens: "checked-out", "project-specific"
Apply these corrections:
-##### Prerequisites +#### Prerequisites-Copy or move the files from the checked out repository to the proper directory on -the Linux machine (_the files must first -be edited to match project specific configuration. Such as the proper RDS instance name in `wait-for-rds.sh`_): +Copy or move the files from the checked-out repository to the proper directory on +the Linux machine (_the files must first +be edited to match project-specific configuration. Such as the proper RDS instance name in `wait-for-rds.sh`_):-The files can also be recreated. Update the files below with project specific configuration then +The files can also be recreated. Update the files below with project-specific configuration thenAlso applies to: 157-159, 169-169
152-152: Consider converting indented code blocks to fenced blocks (optional).Markdown linting prefers fenced code blocks (triple backticks) over indented blocks for consistency. Lines 152, 178, 222, and 260 use indentation. If your markdown renderer supports fenced blocks within container elements, consider converting these for better style compliance.
Verify whether your markdown renderer (likely Hugo/Material for MkDocs based on the frontmatter) handles fenced code blocks within
!!! containeror collapsible block syntax. If so, apply standard fenced block formatting.Also applies to: 178-178, 222-222, 260-260
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
card_data/infrastructure/start-dagster.sh(1 hunks)docs/Infrastructure_Guide/cloud-deployment.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- card_data/infrastructure/start-dagster.sh
🧰 Additional context used
🪛 LanguageTool
docs/Infrastructure_Guide/cloud-deployment.md
[style] ~114-~114: Consider a more concise word here.
Context: ...ting Startup with systemd Optional In order to save on costs, the EC2 and RDS instance...
(IN_ORDER_TO_PREMIUM)
[grammar] ~116-~116: Ensure spelling is correct
Context: ...To automate the starting of the Dagster webservice, systemd, along with a couple of she...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~157-~157: Use a hyphen to join words.
Context: ... Copy or move the files from the checked out repository to the proper directory o...
(QB_NEW_EN_HYPHEN)
[grammar] ~158-~158: Use a hyphen to join words.
Context: ...s must first be edited to match project specific configuration. Such as the prop...
(QB_NEW_EN_HYPHEN)
[grammar] ~169-~169: Use a hyphen to join words.
Context: ...ted. Update the files below with project specific configuration then run the `cat...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
docs/Infrastructure_Guide/cloud-deployment.md
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
130-130: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
152-152: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
178-178: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
222-222: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
260-260: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
| be edited to match project specific configuration. Such as the proper RDS instance name in `wait-for-rds.sh`_): | ||
| ```shell | ||
| cp card_data/card_data/infrastructure/wait-for-rds.sh /home/ubuntu/ |
There was a problem hiding this comment.
🧩 Analysis chain
Verify repository path structure for card_data/card_data/ references.
The copy commands (lines 161, 163) and systemd configuration (lines 188, 190) reference card_data/card_data/infrastructure/, which suggests a nested directory structure. The AI summary mentions card_data/infrastructure/, creating ambiguity about the actual repository structure.
Please verify the actual repository structure by confirming:
- Does the checked-out
card_data/directory contain a nestedcard_data/subdirectory? - Is the path in the documentation
card_data/card_data/infrastructure/correct, or should it becard_data/infrastructure/?
Run the following to inspect the actual structure:
Also applies to: 163-163, 188-188, 190-190
🏁 Script executed:
#!/bin/bash
# Verify the actual directory structure in the repository
fd -t d 'infrastructure' --max-depth 5 card_data/ 2>/dev/null | head -20Length of output: 25
---
🏁 Script executed:
```shell
#!/bin/bash
# Extract the problematic lines from the documentation file
sed -n '161p;163p;188p;190p' docs/Infrastructure_Guide/cloud-deployment.md
Length of output: 285
🏁 Script executed:
#!/bin/bash
# Show broader context around these lines to understand the full issue
sed -n '159,165p;186,192p' docs/Infrastructure_Guide/cloud-deployment.mdLength of output: 496
Fix incorrect path references: remove duplicate card_data/ prefix.
The documentation contains erroneous paths with a duplicated card_data/ directory prefix. The actual repository structure is card_data/infrastructure/, not card_data/card_data/infrastructure/.
Correct the following paths on lines 161, 163, 164 (copy commands):
- Change
card_data/card_data/infrastructure/→card_data/infrastructure/
And on lines 188, 190 (systemd configuration):
- Change
WorkingDirectory=/home/ubuntu/card_data/card_data→WorkingDirectory=/home/ubuntu/card_data - Change
PATH=/home/ubuntu/card_data/card_data/.venv/bin→PATH=/home/ubuntu/card_data/.venv/bin
🤖 Prompt for AI Agents
In docs/Infrastructure_Guide/cloud-deployment.md around lines 161–164 and
188–190, fix incorrect paths that include a duplicated card_data/ prefix: update
the three copy commands (lines 161, 163, 164) to use card_data/infrastructure/
instead of card_data/card_data/infrastructure/, and update the systemd settings
(lines 188 and 190) to use WorkingDirectory=/home/ubuntu/card_data and
PATH=/home/ubuntu/card_data/.venv/bin respectively (remove the extra card_data
segment).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores