Add missing bucket resource definition#599
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
….com:pocketsizefund/pocketsizefund into 06-08-update_documentation_and_minor_cleanup
….com:pocketsizefund/pocketsizefund into 06-08-update_documentation_and_minor_cleanup
….com:pocketsizefund/pocketsizefund into 06-08-update_documentation_and_minor_cleanup
|
""" WalkthroughThe changes comment out the Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Pulumi as Pulumi Infra
participant GCS as Google Cloud Storage
participant IAM as IAM
Dev->>Pulumi: Define grafana_dashboards_bucket
Pulumi->>GCS: Create "grafana-dashboards-bucket"
Pulumi->>IAM: Grant objectAdmin role on grafana-dashboards-bucket to platform service account
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a missing Grafana dashboards bucket resource and refactors monitoring configuration names for consistency.
- Renames
configtoconfigurationand updates Prometheus resource identifiers inmonitoring.py. - Introduces
grafana_dashboards_bucketand its IAM member inbuckets.py. - Temporarily comments out type checks in
.mise.toml.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| infrastructure/monitoring.py | Updated Config variable, renamed Prometheus bucket and volume names, and adjusted empty_dir usage |
| infrastructure/buckets.py | Defined new Grafana dashboards bucket and assigned objectAdmin IAM role |
| .mise.toml | Commented out the uvx ty check task and added a note about hanging failure |
Comments suppressed due to low confidence (1)
infrastructure/buckets.py:24
- [nitpick] The new Grafana dashboards bucket and IAM binding lack automated tests; consider adding a unit or integration test to verify bucket properties and IAM roles.
grafana_dashboards_bucket = storage.Bucket(
Graphite Automations"Assign author to pull request" took an action on this PR • (06/13/25)1 assignee was added to this PR based on John Forstmeier's automation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
infrastructure/monitoring.py (1)
83-96:⚠️ Potential issue
empty_dir=Nonelikely invalid – will generateemptyDir: nullPulumi treats
Noneas an explicit null which Cloud Run rejects. Use an empty object ({}) or omit the field:- name="prometheus-data", - empty_dir=None, + name="prometheus-data", + empty_dir={},Please run
pulumi previewto confirm.
🧹 Nitpick comments (4)
.mise.toml (1)
25-31:Details
❓ Verification inconclusive
Temporary removal of type-checker weakens CI gate & comment typo
Disabling
uvx ty checkremoves the only automated type-safety gate. Leaving it commented may allow regressions to slip through unnoticed.
If the tool hangs, consider:
- Running it with a timeout (
timeout 5m uvx ty check) so CI fails fast instead of hanging.- Moving the step behind an env-flag so local hangs don’t block the pipeline.
- Opening a tracking ticket and adding a TODO here referencing it.
Minor:
hangnig→hanging.-# temporarily commented out ty check due to hangnig failure +# TODO(OPS-123): re-enable after fixing hanging issue +# temporarily commented out ty check due to hanging failure # timeout 5m uvx ty check
🏁 Script executed:
#!/bin/bash # Verify all occurrences of the type-checker invocation in the repo rg -n "uvx ty check" rg -n "ty check"Length of output: 473
Re-enable CI type-check step with timeout & TODO
It looks like the only invocation of
uvx ty checklives in.mise.tomland is currently commented out—removing your CI’s type-safety gate. Please:
- File:
.mise.toml(around lines 25–31)
• Open a tracking ticket (e.g.OPS-123) for the hanging issue.
• Fix the typo “hangnig” → “hanging” and reference the ticket in a TODO.
• Re-enable the step under a timeout or behind an env flag so CI can fail fast.-# temporarily commented out ty check due to hangnig failure +# TODO(OPS-123): re-enable after fixing hanging issue +# temporarily commented out ty check due to hanging failure # timeout 5m uvx ty checkConsider using
timeout 5m uvx ty checkin CI or gating it behind an env flag to prevent local hangs from blocking your pipeline.infrastructure/buckets.py (1)
24-38: Grafana bucket created without lifecycle or versioning – consider hardeningThe dashboards bucket will store configuration that teams may edit over time. Enabling object versioning and a basic retention policy makes rollback trivial and guards against accidental deletions.
grafana_dashboards_bucket = storage.Bucket( "grafana-dashboards-bucket", name=config.require_secret("grafana_dashboards_bucket_name"), location=project.REGION, uniform_bucket_level_access=True, + versioning={"enabled": True}, + retention_policy={"retention_period": 86_400}, # 1 day example )Optional but recommended.
infrastructure/monitoring.py (2)
31-42: Hard-coded YAML inside source – consider externalisingEmbedding a multi-line Prometheus YAML string makes diffing and linting harder. Storing it under
infrastructure/prometheus.yamland loading withPath.read_text()improves maintainability.
45-48: Bucket choice couples config & dashboards
prometheus_configurationis written to the Grafana dashboards bucket. Config files and dashboards have different lifecycles; using a separate config bucket would keep concerns isolated and permissions minimal (principle of least privilege).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
.flox/env/manifest.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
.mise.toml(1 hunks)infrastructure/buckets.py(1 hunks)infrastructure/monitoring.py(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
infrastructure/buckets.py (1)
application/datamanager/src/datamanager/config.py (1)
Bucket(15-24)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run Python tests
🔇 Additional comments (2)
infrastructure/monitoring.py (2)
70-71: Volume name rename LGTMContainer mount now matches volume resource; avoids runtime mount errors.
6-9:Details
✅ Verification successful
Consistent variable name, but ensure no lingering
configusagesYou swapped
config→configuration. Any strayconfig.require_*later in this file (or files that import from here) will raiseNameError. Please grep the repo and update remaining references.
🏁 Script executed:
rg -n '\bconfig\.' infrastructure/monitoring.pyLength of output: 125
No remaining
configvariable references
The onlyconfig.match is inside the Prometheus CLI flag string on line 62, not a code reference. No further changes are required.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
forstmeier
force-pushed
the
06-12-add_missing_bucket_resource_definition
branch
from
6169e96 to
1d8f482
Compare
…n_and_minor_cleanup Update documentation and minor cleanup
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@chrisaddy this will need another 👀 . |
….com:pocketsizefund/pocketsizefund into 06-12-add_missing_bucket_resource_definition
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.mise.toml (1)
25-25: Add TODO for re-enablinguvx ty check
Include a TODO or GitHub issue reference so this lint step isn't permanently disabled once the hanging failure is resolved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/CODE_OF_CONDUCT.md(1 hunks).github/CONTRIBUTING.md(1 hunks).github/SECURITY.md(1 hunks).mise.toml(1 hunks)README.md(1 hunks)todos.md(0 hunks)
💤 Files with no reviewable changes (1)
- todos.md
✅ Files skipped from review due to trivial changes (4)
- .github/SECURITY.md
- .github/CONTRIBUTING.md
- .github/CODE_OF_CONDUCT.md
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run Python quality checks
- GitHub Check: Run Python tests
🔇 Additional comments (1)
.mise.toml (1)
30-30: Assess impact of disabling type lint
Confirm that skipping theuvx ty checkwon’t allow critical type issues to slip through and ensure there’s a clear plan to reinstate it.
forstmeier
dismissed
chrisaddy’s stale review
The merge-base changed after approval.
forstmeier
dismissed
chrisaddy’s stale review
The merge-base changed after approval.
Overview
Changes
Comments
I just added the bucket but I assume we're gonna have some changes if we setup something like Grafana Cloud.
Summary by CodeRabbit
New Features
Chores