feat: automate snapshot creation and scheduling#4938
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary by CodeRabbit
WalkthroughAdds a snapshot creation command, a Makefile target, unit tests for the command flow, and Terraform changes that schedule the task in ECS. ChangesSnapshot Creation Automation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
github-actions
Bot
added
docs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 files
Confidence score: 2/5
- In
backend/apps/owasp/management/commands/owasp_create_snapshot.py, the command referencesSnapshot.Frequency, which is missing on the model, so running the command will raise anAttributeErrorimmediately and block snapshot creation—replace this with the correct model enum/field reference and run the command path before merging. - In
backend/apps/owasp/management/commands/owasp_create_snapshot.py,keyis checked for duplicates but not passed intoSnapshot.objects.create(), so saved rows can get a different generated key and weekly duplicate prevention fails—pass the checkedkeyinto create (or align duplicate logic with generated keys) and add a regression test for weekly dedupe before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/owasp/management/commands/owasp_create_snapshot.py (1)
111-128:⚠️ Potential issue | 🟠 MajorRemove or correct the misleading comment about mirroring save() logic.
The comment at line 115 claims the
generate_key()method "mirrors the logic in the Snapshot model's save() method," but this is inaccurate. The Snapshot model'ssave()method only generates monthly keys usingnow().strftime("%Y-%m")and does not implement the weekly ISO calendar logic thatgenerate_key()provides. Update or remove the misleading comment to reflect the actual implementation difference, and consider whether thesave()method should also support the weekly frequency format to ensure consistency across key generation paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py` around lines 111 - 128, The comment in the generate_key() method at line 115 claims it mirrors the logic in the Snapshot model's save() method, but this is inaccurate since the save() method only generates monthly keys using strftime("%Y-%m") and does not implement the weekly ISO calendar logic that generate_key() provides. Remove or correct this misleading comment to accurately reflect that generate_key() is a separate implementation that supports both weekly and monthly frequency formats, which differs from what the save() method currently does.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py`:
- Around line 111-128: The comment in the generate_key() method at line 115
claims it mirrors the logic in the Snapshot model's save() method, but this is
inaccurate since the save() method only generates monthly keys using
strftime("%Y-%m") and does not implement the weekly ISO calendar logic that
generate_key() provides. Remove or correct this misleading comment to accurately
reflect that generate_key() is a separate implementation that supports both
weekly and monthly frequency formats, which differs from what the save() method
currently does.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6649067-6fe6-44fe-991e-4723c5847c00
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_create_snapshot.pybackend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.py
HarshitVerma109
added
the
gsoc2026:harshitverma109
HarshitVerma109
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
9949320
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/community-snapshots #4938 +/- ##
===============================================================
- Coverage 98.82% 98.82% -0.01%
===============================================================
Files 541 542 +1
Lines 17289 17336 +47
Branches 2496 2500 +4
===============================================================
+ Hits 17086 17132 +46
- Misses 88 89 +1
Partials 115 115
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
Hi @Arkadii, could you please review and merge this PR when you have a moment? |
Signed-off-by: Harsh <harshit1092004@gmail.com>
HarshitVerma109
force-pushed
the
feature/automate-snapshots
branch
from
9949320 to
3cf5c1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_create_snapshot.py (1)
28-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the
Frequencyenum values instead of enumerating choices.
Snapshot.Frequency.valuesalready exposes the full set of valid values; listing members manually duplicates the source of truth and can drift if a frequency is added.♻️ Suggested change
- choices=[Snapshot.Frequency.WEEKLY, Snapshot.Frequency.MONTHLY], + choices=Snapshot.Frequency.values,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py` at line 28, The `owasp_create_snapshot.py` command is hardcoding the `choices` for `Snapshot.Frequency` instead of using the enum’s source of truth. Update the argument definition in the snapshot command to use `Snapshot.Frequency.values` directly so the valid frequencies stay in sync with the enum, and keep the change localized to the snapshot creation command logic that references `Snapshot.Frequency`.backend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.py (1)
75-81: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssertion will need
frequency="monthly"once the command bug is fixed.This
assert_called_once_with(...)omitsfrequency, matching the current (incorrect) behavior flagged inowasp_create_snapshot.pylines 62-68. Update it to includefrequency="monthly"(andfrequency="weekly"in the weekly test) when applying that fix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.py` around lines 75 - 81, The snapshot creation test is asserting the current buggy behavior and is missing the expected frequency argument. Update the `assert_called_once_with` in `owasp_create_snapshot_test` to include the `frequency` field that the fixed `owasp_create_snapshot` command should pass through, using `frequency="monthly"` for this monthly case and `frequency="weekly"` in the weekly test, so the test matches the corrected `Snapshot.objects.create` call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py`:
- Around line 62-68: The snapshot creation in owasp_create_snapshot is missing
the frequency value, so records created by Snapshot.objects.create end up with
the default weekly frequency even when the command runs with monthly. Update the
command’s snapshot creation logic to pass the current frequency into
Snapshot.objects.create, and adjust the related test
test_handle_creates_snapshot_monthly so it expects frequency to be set rather
than omitted.
---
Duplicate comments:
In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py`:
- Line 28: The `owasp_create_snapshot.py` command is hardcoding the `choices`
for `Snapshot.Frequency` instead of using the enum’s source of truth. Update the
argument definition in the snapshot command to use `Snapshot.Frequency.values`
directly so the valid frequencies stay in sync with the enum, and keep the
change localized to the snapshot creation command logic that references
`Snapshot.Frequency`.
In
`@backend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.py`:
- Around line 75-81: The snapshot creation test is asserting the current buggy
behavior and is missing the expected frequency argument. Update the
`assert_called_once_with` in `owasp_create_snapshot_test` to include the
`frequency` field that the fixed `owasp_create_snapshot` command should pass
through, using `frequency="monthly"` for this monthly case and
`frequency="weekly"` in the weekly test, so the test matches the corrected
`Snapshot.objects.create` call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a5ac54fb-0bfa-4cd6-8681-719a4bab4c8d
📒 Files selected for processing (6)
backend/apps/owasp/Makefilebackend/apps/owasp/management/commands/owasp_create_snapshot.pybackend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.pyinfrastructure/modules/tasks/README.mdinfrastructure/modules/tasks/main.tfinfrastructure/modules/tasks/variables.tf
Signed-off-by: Harsh <harshit1092004@gmail.com>
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/owasp/management/commands/owasp_create_snapshot.py (1)
53-69: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake duplicate handling atomic.
Lines 55-69 still do a check-then-create sequence. Two overlapping runs can both pass
.exists(), and the loser will fail on the uniqueSnapshot.keyconstraint instead of behaving idempotently. Please switch this toget_or_create(..., defaults=...)or catchIntegrityErrorand treat it as “already exists”.Suggested change
- if Snapshot.objects.filter(key=key).exists(): - self.stdout.write( - self.style.WARNING(f"Snapshot with key '{key}' already exists, skipping creation") - ) - logger.info("Snapshot with key '%s' already exists, skipping", key) - return - - snapshot = Snapshot.objects.create( - key=key, - frequency=frequency, - start_at=start_at, - end_at=end_at, - title=self.generate_title(start_at, frequency), - status=Snapshot.Status.PENDING, - ) + snapshot, created = Snapshot.objects.get_or_create( + key=key, + defaults={ + "frequency": frequency, + "start_at": start_at, + "end_at": end_at, + "title": self.generate_title(start_at, frequency), + "status": Snapshot.Status.PENDING, + }, + ) + if not created: + self.stdout.write( + self.style.WARNING(f"Snapshot with key '{key}' already exists, skipping creation") + ) + logger.info("Snapshot with key '%s' already exists, skipping", key) + return🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py` around lines 53 - 69, The duplicate snapshot handling in the snapshot creation command is still doing a non-atomic exists-then-create check, which can race under overlapping runs. Update the logic in the management command method that builds the Snapshot (using generate_key, generate_title, and Snapshot.objects.create) to use an atomic get_or_create with defaults, or catch IntegrityError around the create and treat that case as already existing. Keep the existing skip/log behavior for the duplicate path so repeated runs remain idempotent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/apps/owasp/management/commands/owasp_create_snapshot.py`:
- Around line 53-69: The duplicate snapshot handling in the snapshot creation
command is still doing a non-atomic exists-then-create check, which can race
under overlapping runs. Update the logic in the management command method that
builds the Snapshot (using generate_key, generate_title, and
Snapshot.objects.create) to use an atomic get_or_create with defaults, or catch
IntegrityError around the create and treat that case as already existing. Keep
the existing skip/log behavior for the duplicate path so repeated runs remain
idempotent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ff6ac96a-5841-46d1-be5d-f56e9006dcb9
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_create_snapshot.pybackend/tests/unit/apps/owasp/management/commands/owasp_create_snapshot_test.py
2 participants
Proposed change
Resolves #4744
This PR fully automates the OWASP snapshot lifecycle so we no longer have to create them manually through the admin panel. It safely calculates the correct date ranges, prevents duplicate entries, and schedules the entire pipeline to run completely on its own every Monday morning.
Checklist
make check-testlocally: all warnings addressed, tests passed