[ENG-3815] feat(microsoft): Impl Create Subscription#2878
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Cobalt0s
force-pushed
the
cobalt0s/microsoft-webhook-verifier
branch
from
97fae5b to
50c6650
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-subscribe-create
branch
from
3b926aa to
1289e81
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-subscribe-create
branch
from
1289e81 to
f145e5c
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-webhook-verifier
branch
from
50c6650 to
92c0bef
Compare
Cobalt0s
changed the base branch from
cobalt0s/microsoft-webhook-verifier
to
graphite-base/2878
Cobalt0s
force-pushed
the
graphite-base/2878
branch
from
92c0bef to
bfa981f
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-subscribe-create
branch
from
f145e5c to
f9b7354
Compare
| subscribe | ||
| subscriber | ||
| ./subscribe | ||
| ./subscriber |
There was a problem hiding this comment.
I am not sure what is the reason for ignoring these and where exactly this data is stored. The directory names are good and shouldn't be excluded.
Cobalt0s
force-pushed
the
graphite-base/2878
branch
from
bfa981f to
d3bd6c1
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-subscribe-create
branch
from
f9b7354 to
920f377
Compare
jlimatampersand
left a comment
jlimatampersand
left a comment
There was a problem hiding this comment.
There are too many unnecessary structs and functions that are beyond the scope of what is necessary to implement Subscribe.
Please remember this is an open-source project that many different contributors with various experiences and familiarity contribute to.
Any new introduction of internal packages and data utils becomes an immediate huddle and a learning curve for every contributor, making onboarding harder and adding complexity.
Please try using native packages unless you really need to use wrapper packages.
| // | ||
| // It uses a mock HTTP client and overrides the base URL to point to a test server. | ||
| // A fixed clock is injected to ensure deterministic behavior in tests. | ||
| func constructTestStrategy(serverURL string) (*Strategy, error) { |
There was a problem hiding this comment.
this should be in test folder
| } | ||
|
|
||
| return &common.SubscriptionResult{ | ||
| Result: Output{}, |
There was a problem hiding this comment.
There is no need to have an empty struct type as a placeholder. Result type is any, so we don't even need to populate it as it will default to nil.
| // It is part of the connector's input/output contract: the caller expresses intent | ||
| // with State, and the connector returns the same type reflecting what actually | ||
| // happened (which may differ on failure or rollback). | ||
| State map[ObjectName]common.ObjectEvents |
There was a problem hiding this comment.
is this wrapper layer needed? Unless there is a strong reason, I would like to keep each provider addition to a minimal context. There is no new variable, method, defined for this State struct, so I would remove this.
|
|
||
| // Clock provides the current time, enabling deterministic testing of time-dependent logic. | ||
| // Implementations include production (real time) and test (fixed time) variants. | ||
| type Clock interface { |
There was a problem hiding this comment.
I am strongly against adding new components that are just wrappers of native packages.
This only introduces onboarding complexity and load.
Please remove this type and just the native package.
| objectNames := datautils.FromMap(params.SubscriptionEvents).Keys() | ||
|
|
||
| return &common.SubscriptionResult{ | ||
| Result: Output{}, |
There was a problem hiding this comment.
nil is sufficient. or just leave it empty.
| Right R | ||
| } | ||
|
|
||
| func NewPair[L, R any](left L, right R) *Pair[L, R] { |
There was a problem hiding this comment.
unnecessary util func. Please remove.
|
|
||
| // Dependent services. | ||
| batchStrategy *batch.Strategy | ||
| clock components.Clock |
There was a problem hiding this comment.
is this necessary? if this is just useful for testing, I would just opt for the native time package.
Cobalt0s
force-pushed
the
graphite-base/2878
branch
from
d3bd6c1 to
de29855
Compare
Cobalt0s
force-pushed
the
cobalt0s/microsoft-subscribe-create
branch
from
920f377 to
8adbb04
Compare
Cobalt0s
changed the base branch from
graphite-base/2878
to
cobalt0s/subscribe-scripts
2 participants
Description
Implements Microsoft create-subscription support. Also, using the new scripting infrastructure, adds integration tests and unit tests.
Key changes
Subscription implementation
Implements
SubscriptionCreatorusing thesubscriberstrategy (this strategy will implement update & delete in subsequent PRs).Microsoft-specific types
Exports
SubscribeRequestandSubscribeResponsefor server integration.Script integration
Uses the new scripts to validate real scenarios:
Unit testing
Adds
testroutines.CreateSubscriptionfor validating subscription creation logic.Time abstraction
Introduces
components.Clock(RealClockandFixedClock) to support deterministic testing.Live Tests
Outlook Calendar Events
Outlook Mail Messages