Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add GET {aggregator_api}/ → AggregatorApiConfig #1646

Merged
merged 3 commits into from
Jul 31, 2023
Merged

add GET {aggregator_api}/ → AggregatorApiConfig #1646

merged 3 commits into from
Jul 31, 2023

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Jul 26, 2023 •
edited
Loading

Pairs with divviup/divviup-api#340

Explanation of changes:

  • Previously the aggregator-api handler was unconditionally built and discarded, but as of this PR, we require the aggregator-api config in order to build the aggregator-api handler. This seems more generally correct and adaptable to other configuration keys that are mandatory if and only if aggregator-api is enabled
  • Previously, the BinaryContext was partially moved in order to move the non-clone datastore into an Arc, but that did not work well with extracting behvior to a function. To address this, we now destructure the BinaryContext and explicitly take or borrow fields from it as needed.
  • Box::pin(async {}) was replaced with Box::pin(std::future::ready(()))
  • This PR puts Config back into the Conn in auth_check in order to use it in the get_config endpoint

@jbr jbr force-pushed the aggregator-api-config-route branch 3 times, most recently from 1afc43c to 4b6799c Compare July 26, 2023 22:08
jbr
jbr commented Jul 26, 2023
View reviewed changes
aggregator_core/src/lib.rs Outdated Show resolved Hide resolved
@jbr jbr force-pushed the aggregator-api-config-route branch 9 times, most recently from 7a1abd5 to 3eb378d Compare July 27, 2023 18:56
@jbr jbr mentioned this pull request Jul 27, 2023
Merged
@jbr jbr marked this pull request as ready for review July 27, 2023 19:47
@jbr jbr requested a review from a team as a code owner July 27, 2023 19:47
inahga
inahga reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@inahga inahga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job overall, some minor comments

aggregator_api/src/lib.rs Show resolved Hide resolved
aggregator_api/src/lib.rs Show resolved Hide resolved
aggregator_api/src/lib.rs Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
inahga
inahga approved these changes Jul 28, 2023
View reviewed changes
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
divergentdave
divergentdave approved these changes Jul 28, 2023
View reviewed changes
aggregator_core/src/lib.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
@tgeoghegan tgeoghegan mentioned this pull request Jul 28, 2023
Open
tgeoghegan
tgeoghegan requested changes Jul 28, 2023
View reviewed changes
Copy link
Contributor

@tgeoghegan tgeoghegan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a rebase and some compilation error fixes but looks sound

Cargo.lock Outdated Show resolved Hide resolved
aggregator/src/bin/aggregator.rs Show resolved Hide resolved
aggregator/src/bin/aggregator.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
aggregator_api/src/lib.rs Outdated Show resolved Hide resolved
@tgeoghegan tgeoghegan mentioned this pull request Jul 28, 2023
Merged
@jbr jbr force-pushed the aggregator-api-config-route branch from 822bd37 to 59ccd7e Compare July 28, 2023 19:46
@jbr @divergentdave
Update aggregator_core/src/lib.rs
21b884b 
Co-authored-by: David Cook <divergentdave@gmail.com>
@jbr jbr force-pushed the aggregator-api-config-route branch from 0797a23 to 991b894 Compare July 28, 2023 21:15
@jbr jbr force-pushed the aggregator-api-config-route branch 2 times, most recently from 2d352c7 to 9f910cf Compare July 28, 2023 22:26
@jbr jbr force-pushed the aggregator-api-config-route branch from 9f910cf to 6dd7f76 Compare July 28, 2023 23:25
@tgeoghegan tgeoghegan merged commit 27bbe8c into divviup:main Jul 31, 2023
7 checks passed
tgeoghegan added a commit that referenced this pull request Jul 31, 2023
@tgeoghegan
in-cluster test: deal with agg config API
16e2b7a 
With the latest `divviup-api` and `janus` changes ([1], [2]), we no
longer tell `divviup-api` what DAP roles an aggregator supports nor the
URL of its DAP API. Those configuration items are instead discovered
during aggregator pairing. This updates the `divviup-api` client used in
the `in-cluster` integration tests accordingly.

[1]: #1646
[2]: divviup/divviup-api#340
@tgeoghegan tgeoghegan mentioned this pull request Jul 31, 2023
Merged
tgeoghegan added a commit that referenced this pull request Aug 1, 2023
@tgeoghegan
in-cluster test: deal with agg config API (#1665)
f92e176 
With the latest `divviup-api` and `janus` changes ([1], [2]), we no
longer tell `divviup-api` what DAP roles an aggregator supports nor the
URL of its DAP API. Those configuration items are instead discovered
during aggregator pairing. This updates the `divviup-api` client used in
the `in-cluster` integration tests accordingly.

[1]: #1646
[2]: divviup/divviup-api#340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@divergentdave divergentdave divergentdave approved these changes

@inahga inahga inahga approved these changes

@tgeoghegan tgeoghegan tgeoghegan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jbr @divergentdave @tgeoghegan @inahga