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.

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

[4/n] Partial refactor of worker and admin role configs #1328

Merged
merged 1 commit into from
Mar 28, 2024
Merged

Conversation

AhmedSoliman
Copy link
Contributor

@AhmedSoliman AhmedSoliman commented Mar 27, 2024

[4/n] Partial refactor of worker and admin role configs

More re-org and cleanup. We need to move away from the Options::build() style:

  • We should treat configuration as data-only structures that won't be consumed on service creation
  • We pave the way for updateable config, components will own a "projection" to the config in the future instead of consuming it on startup

This is not the final structure, many changes are transitional

@AhmedSoliman AhmedSoliman changed the title Partial refactor of worker and admin role configs [4/n] Partial refactor of worker and admin role configs Mar 27, 2024
Copy link

github-actions bot commented Mar 27, 2024

Test Results

 92 files  ±0   92 suites  ±0   10m 20s ⏱️ -5s
 77 tests  - 2   73 ✅  - 4  2 💤 ±0  2 ❌ +2 
201 runs   - 4  191 ✅  - 8  6 💤 ±0  4 ❌ +4 

For more details on these failures, see this check.

Results for commit dd47f77. ± Comparison against base commit 1a47f34.

This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.
dev.restate.e2e.JavaKafkaIngressTest ‑ handleEventInCounterService(URL, int, IngressClient)
dev.restate.e2e.JavaKafkaIngressTest ‑ handleEventInEventHandler(URL, int, IngressClient)
dev.restate.e2e.NodeKafkaIngressTest ‑ handleEventInCounterService(URL, int, IngressClient)
dev.restate.e2e.NodeKafkaIngressTest ‑ handleEventInEventHandler(URL, int, IngressClient)
dev.restate.e2e.JavaKafkaIngressTest ‑ initializationError
dev.restate.e2e.NodeKafkaIngressTest ‑ initializationError

♻️ This comment has been updated with latest results.

@AhmedSoliman AhmedSoliman marked this pull request as ready for review March 27, 2024 16:50
Copy link
Contributor

@tillrohrmann tillrohrmann left a comment

Choose a reason for hiding this comment

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

With every commit our configuration story becomes a bit nicer. Well done :-) +1 for merging.


/// todo: remove.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think admin_address is no longer used. So feel free to just remove it.

@@ -46,6 +46,14 @@ impl ServiceClient {
pub(crate) fn new(http: HttpClient, lambda: LambdaClient) -> Self {
Self { http, lambda }
}

pub fn from_options(options: Options, assume_role_cache_mode: AssumeRoleCacheMode) -> Self {
let (http, lambda) = options.dissolve();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, learned again something :-)

More re-org and cleanup. We need to move away from the `Options::build()` style:
- We should treat configuration as data-only structures that won't be consumed on service creation
- We pave the way for updateable config, components will own a "projection" to the config in the future instead of consuming it on startup

This is not the final structure, many changes are transitional
@AhmedSoliman AhmedSoliman merged commit 5984b44 into main Mar 28, 2024
8 of 10 checks passed
@AhmedSoliman AhmedSoliman deleted the pr1328 branch March 28, 2024 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants