Add notification subscription support to CLI#2528
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to add notification subscription support to the Keboola CLI, enabling users to manage notification subscriptions as code. The implementation includes model objects, mappers for local file operations, remote API integration, and documentation.
Changes:
- Adds notification domain model with key, state, and manifest types
- Implements local load/save mappers for notification meta.json files
- Adds remote operations for loading, creating, and deleting notifications via SDK
- Integrates notifications into registry and state management
- Adds comprehensive documentation (253 lines)
Reviewed changes
Copilot reviewed 24 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/pkg/model/keys.go | Adds NotificationKey type with Level=4, parent=ConfigKey |
| internal/pkg/model/keys_test.go | Unit tests for NotificationKey methods |
| internal/pkg/model/notification.go | Notification domain model with Event, Filters, Recipient, ExpiresAt fields |
| internal/pkg/model/notificationstate.go | ObjectState implementation for notifications (139 lines) |
| internal/pkg/model/notificationmanifest.go | Manifest record type for notifications |
| internal/pkg/model/manifest.go | Adds Notifications slice to ConfigManifest |
| internal/pkg/model/object.go | Adds NotificationsFrom methods to ObjectStates/Objects interfaces |
| internal/pkg/mapper/notification/notification.go | Base mapper struct |
| internal/pkg/mapper/notification/local_save.go | Creates notifications/{id}/meta.json files |
| internal/pkg/mapper/notification/local_load.go | Loads notifications from filesystem subdirectories |
| internal/pkg/state/remote/notification.go | Remote load/create/delete operations |
| internal/pkg/state/remote/manager.go | Integrates notification special handling and loadNotifications call |
| internal/pkg/state/registry/registry.go | Adds Notifications/NotificationsFrom query methods |
| internal/pkg/state/registry/proxy.go | Adds NotificationsFrom with state type filtering |
| internal/pkg/state/manifest/records.go | Handles NotificationKey in CreateOrGetRecord |
| internal/pkg/project/mappers.go | Registers notification mapper |
| internal/pkg/service/cli/docs/notifications.md | 253-line user documentation |
| internal/pkg/service/cli/CLI_CONTEXT.md | Quick reference section for notifications |
| go.mod, go.sum | Updates SDK to pseudo-version with notification support |
| test/cli/pull/notifications/pull-config-level/* | E2E test files for pull operation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
3bd7012 to
c4663c1
Compare
...cations-create/in/main/extractor/ex-generic-v2/my-config/notifications/sub-alert/config.json
Show resolved
Hide resolved
...ations-create/out/main/extractor/ex-generic-v2/my-config/notifications/sub-alert/config.json
Show resolved
Hide resolved
- Fix manifest ID not updated after notification create/update:
Set notificationState.NotificationManifest.ID = created.ID in the
WithOnSuccess callback so the manifest correctly persists the
API-assigned subscription ID after push.
- Write auto-filters back to local files after push:
Update notification.Filters = created.Filters in callback so the
API-assigned filters (including auto-populated branch.id,
job.component.id, job.configuration.id) are written back to
config.json after push via a local save in remote.UnitOfWork.Invoke().
- Remove unnecessary custom JSON marshaling from ConfigManifestWithRows:
Add MarshalJSON to NotificationManifest that outputs only {id, path},
removing the need for NotificationManifestRef wrapper struct and the
custom ConfigManifestWithRows.MarshalJSON method.
- Remove docs/notifications.md as requested by reviewer.
- Remove notification-specific subsections from CLI_OBJECT_LIFECYCLE.md
(Auto-Filter Behavior and Delete-Then-Create for Updates sections).
- Fix E2E test manifests to use %A wildcard for API-assigned IDs.
- Update E2E test out config.json files to include auto-filters.
- Add new E2E test notifications-create-with-filters to verify that
user-specified filters are combined with auto-populated filters.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
test/cli/push/notifications-create-with-filters/out/.keboola/manifest.json
Outdated
Show resolved
Hide resolved
...-with-filters/out/main/extractor/ex-generic-v2/my-config/notifications/sub-alert/config.json
Outdated
Show resolved
Hide resolved
- Fix manifest ID not updated after notification create/update:
Set notificationState.NotificationManifest.ID = created.ID in the
WithOnSuccess callback so the manifest correctly persists the
API-assigned subscription ID after push.
- Write auto-filters back to local files after push:
Update notification.Filters = created.Filters in callback so the
API-assigned filters (including auto-populated branch.id,
job.component.id, job.configuration.id) are written back to
config.json after push via a local save in remote.UnitOfWork.Invoke().
- Remove unnecessary custom JSON marshaling from ConfigManifestWithRows:
Add MarshalJSON to NotificationManifest that outputs only {id, path},
removing the need for NotificationManifestRef wrapper struct and the
custom ConfigManifestWithRows.MarshalJSON method.
- Remove docs/notifications.md as requested by reviewer.
- Remove notification-specific subsections from CLI_OBJECT_LIFECYCLE.md
(Auto-Filter Behavior and Delete-Then-Create for Updates sections).
- Fix E2E test manifests to use %A wildcard for API-assigned IDs.
- Update E2E test out config.json files to include auto-filters.
- Add new E2E test notifications-create-with-filters to verify that
user-specified filters are combined with auto-populated filters.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
...-with-filters/out/main/extractor/ex-generic-v2/my-config/notifications/sub-alert/config.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Review: Add notification subscription support to CLI
The feature design is solid — the pull/push/diff lifecycle is well-thought-out, the documentation (CLI_OBJECT_LIFECYCLE.md) is excellent, and the mapper/state patterns are consistent with the existing codebase. A few issues need to be addressed before this is ready to merge.
Blockers
1. Compile error in retry_test.go (see inline comment)
new(utctime.MustParse(...)) is invalid Go — new takes a type, not a value expression. This breaks compilation of an unrelated test file. Revert to ptr.Ptr(utctime.MustParse(...)).
2. Pseudo-version SDK dependency (see inline comment on go.mod)
The PR uses v2.12.1-0.20260224141819-0b0f6e9b37d3, a pseudo-version. CLAUDE.md explicitly prohibits this. A tagged SDK release must be available before this PR is merged.
Bugs
3. Filter operator not checked when matching parent config
In loadNotifications(), the code matches a subscription to its parent config by checking filter.Field == "job.configuration.id" without checking filter.Operator. A subscription with job.configuration.id != "my-config" (not-equals) would be incorrectly attributed to config my-config. See inline comment on remote/notification.go.
4. ValidateNotificationFilters is dead code
The validation function is only called from its own test file — it is never invoked during push, pull, or local-load operations. Users with invalid filter field names will only discover the problem via the API's 400 response, which is caught by the less helpful string-matching error handler. The filter validation should be wired into the actual operation path (e.g., called from the local load mapper or from a Validate() hook on the notification model).
5. Potential duplicate filters on push
After the first push, the API filters (including the auto-populated job.configuration.id) are written back to the local config.json. On a subsequent push, createNotificationRequest() prepends those same fields as auto-filters and then appends notification.Filters (which already contains them), resulting in duplicate filters being sent to the API. Add a deduplication step (skip auto-filters whose field already appears in notification.Filters). See inline comment.
Code quality
6. Non-deterministic findSimilarFieldNames output
The function iterates over deprecatedNotificationFilterFieldNames() (a map) without sorting, violating the project convention: "When iterating over maps, always sort results before output." This makes error message suggestions non-deterministic. See inline comment.
7. Repeated allocation of validation helpers
validNotificationFilterFields() and deprecatedNotificationFilterFieldNames() create a new slice/map on every call. They are called multiple times per validation invocation. Promote them to package-level var declarations. See inline comment.
8. Fragile HTTP error detection
The WithOnError callback uses strings.Contains(errMsg, "400") to detect HTTP 400 errors. The literal "400" can appear in notification IDs, config IDs, or filter values in the error message, causing false positives. Prefer checking the error's status code directly (if the SDK exposes it). See inline comment.
9. Path construction duplicated between manifest/file.go and the naming generator
The backwards-compatibility fallback in records() constructs a path with fmt.Sprintf("notifications/sub-%s", notification.ID), duplicating the same format string that lives in Generator.NotificationPath(). See inline comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 96 out of 167 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
19ace45 to
42b5c75
Compare
Address all review comments from PR #2528, add missing test coverage, and fix lint/formatting issues: - SDK integration: use SDK CleanProject (CleanAllNotificationSubscriptionsRequest) for test cleanup instead of manual deletion - Add E2E test documenting push behavior with unsupported custom filter (API returns HTTP 400 and local files remain unchanged) - Address review comments: field naming, error handling, code structure - Fix push operation: add persist support, fix stale key removal, preserve folder paths - Fix DeleteObject: clean up manifest entry and clear remote state on delete - Fix manifest: move Notifications field after Rows in ConfigManifestWithRows - Fix E2E tests: correct test expectations for push/pull/filter scenarios - Fix encrypt E2E test manifest: correct naming fields - Lint: gofumpt, go mod tidy, golangci-lint, import ordering
0308fbc to
6750de9
Compare
Address all review comments from PR #2528, add missing test coverage, and fix lint/formatting issues: - SDK integration: use SDK CleanProject (CleanAllNotificationSubscriptionsRequest) for test cleanup instead of manual deletion - Add E2E test documenting push behavior with unsupported custom filter (API returns HTTP 400 and local files remain unchanged) - Address review comments: field naming, error handling, code structure - Fix push operation: add persist support, fix stale key removal, preserve folder paths - Fix DeleteObject: clean up manifest entry and clear remote state on delete - Fix manifest: move Notifications field after Rows in ConfigManifestWithRows - Fix E2E tests: correct test expectations for push/pull/filter scenarios - Fix encrypt E2E test manifest: correct naming fields - Lint: gofumpt, go mod tidy, golangci-lint, import ordering
6750de9 to
7f63cc7
Compare
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ 1. Remote Load (remote/notification.go) │ | ||
| │ - Fetch from API │ | ||
| │ - u.loadObject(notification) creates Manifest + State │ | ||
| │ - loadObject sets Remote = Local = notification (copy) │ | ||
| │ - Set parent path on manifest │ | ||
| └─────────────────────────────────────────────────────────────┘ | ||
| ↓ | ||
| ┌─────────────────────────────────────────────────────────────┐ | ||
| │ 2. MapBeforeLocalSave (mapper/notification/local_save.go) │ | ||
| │ - Called ONCE PER NOTIFICATION (not per config) │ | ||
| │ - Writes config.json: {event, filters, recipient} │ | ||
| │ - Writes meta.json: {} │ | ||
| └─────────────────────────────────────────────────────────────┘ |
There was a problem hiding this comment.
these ascii art boxes look a bit malformed, do we need them?
and do we need this document? if yes, does it need to be so long?
jachym-tousek-keboola
left a comment
There was a problem hiding this comment.
Nothing looks immediately suspicious except the super long AI generated documents. But this is quite a bit outside of my area of expertise to be honest.
Add support for notification subscriptions as a first-class CLI object: - Domain model: Notification, NotificationState, NotificationManifest with full Object/ObjectState/ObjectManifest interface implementations - Keys: NotificationKey hierarchy (BranchKey → ConfigKey → NotificationKey) - Mapper: notification mapper integrated into the CLI pipeline for loading, transforming, and persisting notification subscriptions - Remote operations: load notifications from Keboola Storage API and push changes
Add comprehensive test coverage for notification subscription support: - Test project utilities: CreateNotificationSubscription, ListNotificationSubscriptions, DeleteNotificationSubscription helpers; fixture support in testproject package - E2E tests: push and pull workflows including create, update, delete scenarios - Unit tests: notification mapper, naming generator (format/parse roundtrip), and validation logic
Add comprehensive documentation covering how CLI objects (configs, rows, notifications) are created, loaded, and saved throughout the lifecycle. Documents State vs Manifest distinction, Registry.Set() rules, mapper timing, and Pull/Push operation flows.
Address all review comments from PR #2528, add missing test coverage, and fix lint/formatting issues: - SDK integration: use SDK CleanProject (CleanAllNotificationSubscriptionsRequest) for test cleanup instead of manual deletion - Add E2E test documenting push behavior with unsupported custom filter (API returns HTTP 400 and local files remain unchanged) - Address review comments: field naming, error handling, code structure - Fix push operation: add persist support, fix stale key removal, preserve folder paths - Fix DeleteObject: clean up manifest entry and clear remote state on delete - Fix manifest: move Notifications field after Rows in ConfigManifestWithRows - Fix E2E tests: correct test expectations for push/pull/filter scenarios - Fix encrypt E2E test manifest: correct naming fields - Lint: gofumpt, go mod tidy, golangci-lint, import ordering
- Remove unnecessary empty line before return in remote/manager.go - Add expiration support to createNotificationRequest: if notification.ExpiresAt is set, pass it to the API using WithAbsoluteExpiration (RFC3339) or WithRelativeExpiration (relative duration string) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Check filter operator (==) when matching subscription to parent config,
preventing false attribution for inequality filters on job.configuration.id
- Replace fragile strings.Contains("400") error detection with proper
errors.As(err, &NotificationError) + StatusCode() == 400 check
- Call ValidateNotificationFilters from MapAfterLocalLoad so invalid filter
field names are caught at load time instead of being dead code
- Sort suggestions in findSimilarFieldNames for deterministic output
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- notification.go: use composite "branchID/configID" key in configsByKey map to prevent collisions when two branches share the same config ID; also collect branch.id filter value alongside job.configuration.id before doing the lookup, require both for a correct match - push.go: add nil guard for v.Remote in parentExists() before dereferencing the NotificationState's Remote pointer - generator.go: remove spurious trailing quote from NotificationPath panic message - mapper.go: correct LocalSaveMapper comment — parent-child manifest relationships (ConfigManifest.Notifications) are populated by setRecords() during manifest load, not by MapBeforeLocalSave - CLI_CONTEXT.md: fix two inaccuracies — notifications are stored in config.json (not meta.json), and filter operators are ==, !=, >, <, >=, <= (not eq, ne, gt, lt, ge, le) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-go v2.13.1 - revert(json): remove SetEscapeHTML(false) from Encode function - test(transport): skip TestTransportBiggerData_KCP on Windows — KCP data corruption under high UDP throughput is a kcp-go/Windows interaction, not application code - chore(deps): bump keboola-sdk-go/v2 to v2.13.1 and go-utils to v1.4.0; transitive upgrades (AWS SDK v2, Azure, GCP, gocloud.dev) are minimum-version requirements of the new SDK; drops aws-sdk-go v1 and jmespath Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
7f63cc7 to
d0f0b56
Compare
Summary
Implements full notification subscription support in the CLI, enabling users to manage Keboola notification subscriptions as code alongside their configurations.
File Structure
Manifest Format
{ "notifications": [ {"id": "%API_ASSIGNED_ID%", "path": "notifications/sub-alert"} ] }Key Design Decisions
job.configuration.idfilter (auto-populated on push).branch.id,job.component.id,job.configuration.idare injected automatically from the parent config context; user-defined filters are merged in.E2E Tests
push/notifications-create— create a new notificationpush/notifications-update— update (delete + create) an existing notificationpush/notifications-delete— delete a notificationpush/notifications-create-with-filters— create with user-defined filtersDependencies
Uses SDK with notification support:
github.com/keboola/keboola-sdk-go/v2 v2.12.1-0.20260223100110-432345f77343A tagged SDK release should be used once available.
🤖 Generated with Claude Code