-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update EventGroup resource pattern in Reporting v2 API. #1064
Conversation
9b3591a
to
2e5c8b3
Compare
1201c39
to
23a9fb3
Compare
23a9fb3
to
ff4e03b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 13 of 14 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @riemanli)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 14 of 14 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportingSetsService.kt
line 517 at r1 (raw file):
return InternalReportingSetKt.primitive { eventGroupKeys += source.cmmsEventGroupsList.map { cmmsEventGroup ->
How do we make sure that the measurement consumer has the permission to those event groups? Or is that their responsibility to maintain a valid set of event groups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @riemanli)
src/main/kotlin/org/wfanet/measurement/reporting/service/api/v2alpha/ReportingSetsService.kt
line 517 at r1 (raw file):
Previously, riemanli (Rieman) wrote…
How do we make sure that the measurement consumer has the permission to those event groups? Or is that their responsibility to maintain a valid set of event groups?
It's up to the MC to put in EventGroups that belong to them, the same as they would when using the CMMS API. Generally they'd only have their own EGs that they got from calling ListEventGroups.
Ultimate enforcement would happen during Requisition fulfillment. An EDP should refuse Requisitions for an MC if any EventGroups don't belong to that MC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
e977044
to
85f44d2
Compare
ff4e03b
to
7d73ba4
Compare
cd23432
to
697fce3
Compare
7943b20
to
0e514de
Compare
055c80d
to
af73529
Compare
0e514de
to
21ff462
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 9 of 14 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SanjayVas and @stevenwarejones)
src/main/proto/wfa/measurement/internal/reporting/v2/reporting_set.proto
line 34 at r4 (raw file):
message EventGroupKey { // `DataProvider` ID from the CMMS public API. string cmms_data_provider_id = 1;
Is this being used already? e.g. is the number changing allowed in this case?
Code quote:
1;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Marco-Premier and @stevenwarejones)
src/main/proto/wfa/measurement/internal/reporting/v2/reporting_set.proto
line 34 at r4 (raw file):
Previously, Marco-Premier (marcopremier) wrote…
Is this being used already? e.g. is the number changing allowed in this case?
Yes, it's allowed because this is not yet being used. There aren't yet deployment artifacts for it, so it can't yet be used.
21ff462
to
5797103
Compare
29f7b5c
to
d68c615
Compare
5797103
to
443c1e1
Compare
d68c615
to
8c7781c
Compare
443c1e1
to
1320cd3
Compare
8c7781c
to
31afb2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
31afb2e
to
7e88faa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
7e88faa
to
b7044e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 14 files at r1, 2 of 2 files at r2, 1 of 1 files at r3, 2 of 3 files at r4, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas)
This takes advantage of the secondary resource pattern for EventGroups in the CMMS API.
This takes advantage of the secondary resource pattern for EventGroups in the CMMS API.