implement configuration by file. by Ahmed-Elsaka-JC · Pull Request #595 · eclipse-score/persistency

Ahmed-Elsaka-JC · 2026-10-26T14:05:54Z

adding new support to config by file

github-actions · 2026-10-26T14:07:33Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Starting local Bazel server (8.7.0) and connecting to it...
INFO: Invocation ID: a1a06680-0a9f-4234-aec7-24c77806ec06
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 2 packages loaded
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Loading: 2 packages loaded
    currently loading: 
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)
Analyzing: target //:license-check (3 packages loaded, 0 targets configured)

Analyzing: target //:license-check (83 packages loaded, 14 targets configured)

Analyzing: target //:license-check (99 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (100 packages loaded, 14 targets configured)

Analyzing: target //:license-check (153 packages loaded, 3717 targets configured)

Analyzing: target //:license-check (204 packages loaded, 8002 targets configured)

Analyzing: target //:license-check (207 packages loaded, 11661 targets configured)

Analyzing: target //:license-check (207 packages loaded, 11661 targets configured)

Analyzing: target //:license-check (212 packages loaded, 17771 targets configured)

Analyzing: target //:license-check (212 packages loaded, 17774 targets configured)

Analyzing: target //:license-check (212 packages loaded, 17774 targets configured)

INFO: Analyzed target //:license-check (213 packages loaded, 17903 targets configured).
[12 / 16] JavaToolchainCompileClasses external/rules_java+/toolchains/platformclasspath_classes; 0s disk-cache, processwrapper-sandbox ... (2 actions running)
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 50 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/tools/formatted.txt
[14 / 16] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache, processwrapper-sandbox
INFO: Found 1 target...
Target //tools:license.check.license_check up-to-date:
  bazel-bin/tools/license.check.license_check
  bazel-bin/tools/license.check.license_check.jar
INFO: Elapsed time: 53.757s, Critical Path: 2.49s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/tools/license.check.license_check tools/formatted.txt <args omitted>
[main] INFO Querying Eclipse Foundation for license data for 42 items.
[main] INFO Found 32 items.
[main] INFO Querying ClearlyDefined for license data for 10 items.
[main] INFO Found 10 items.
[main] INFO License information could not be automatically verified for the following content:
[main] INFO 
[main] INFO crate/cratesio/-/lazy_static/1.5.1
[main] INFO 
[main] INFO This content is either not correctly mapped by the system, or requires review.

github-actions · 2026-10-26T14:19:35Z

Documentation preview for this pull request is available at:
pr-595: https://eclipse-score.github.io/persistency/pr-595/

arkjedrz

Review to internal/ files is not thorough yet, since many raised points relate to design.

arkjedrz · 2026-10-27T10:07:59Z

+#include <optional>
+
+/*
+ * This header declares the abstraction over deployment-time "configuration by file" discovery and


Since this is generated - ask Your LLM of choice to rework docs and comments. Following rules seem to be a good first step.

Be concise, use short sentences.

One sentence per line.

If a sentence is too long, try to rephrase it first.

This one's interesting, but turned out to be the most effective. Write as if it was written for an impatient developer that is not using English as a native language.

This is required, because otherwise those docs needlessly meander.

arkjedrz · 2026-10-27T10:14:02Z

+namespace score::mw::per::kvs
+{
+
+class IKvsConfigParser


Please use non-prefixed name for interface, and then suffix Impl for concrete implementation.
This is the already-required approach in baselibs, already followed by logging.

arkjedrz · 2026-10-27T10:14:59Z

+#include <cstddef>
+#include <optional>
+
+/*


Follow new docs format. /// for docs, Doxygen-style @xyz keywords, // for comments.

arkjedrz · 2026-10-27T10:19:57Z

+     * input: the `InstanceId::id` of the KVS instance about to be opened.
+     * behavior: Discovers whether a configuration-by-file document applies at all, and if so
+     *           parses and strictly validates it in full before looking for an entry matching
+     *           `instance_id`. Performs no mutation; intended to be called once per


Performs no mutation -> then the function should be marked as const, and docs require no additional statement.

arkjedrz · 2026-10-27T10:21:10Z

+     * input: the `InstanceId::id` of the KVS instance about to be opened.
+     * behavior: Discovers whether a configuration-by-file document applies at all, and if so
+     *           parses and strictly validates it in full before looking for an entry matching
+     *           `instance_id`. Performs no mutation; intended to be called once per


intended to be called once per -> additional request to LLM, do not state what is the expected usage and even where it is used, this is just a fluff.

arkjedrz · 2026-10-27T11:43:48Z

+        (void)value;
+        const std::string_view key_view = key.GetAsStringView();
+        bool found = false;
+        for (const auto& allowed_key : allowed)


std::any_of, return false quickly if not satisfied.
Do not use single point of return rule.

arkjedrz · 2026-10-27T11:46:49Z

+score::Unexpected KvsConfigParser::fail(ErrorCode code, std::string_view user_message) const
+{
+    const auto error = MakeError(code, user_message);
+    logger_->LogError() << "KVS config:" << error.Message() << error.UserMessage();


logger_ can become an object placed in anonymous namespace, then this function can be in anonymous function as well. Or a KVS-wide internal object, because it's using the same code.

arkjedrz · 2026-10-27T11:47:31Z

+    return fail(ErrorCode::InvalidConfiguration, "unknown need_* value");
+}
+
+/*


cpp is not a place for docs.

arkjedrz · 2026-10-27T11:51:00Z

+
+    /*
+     * Determine the directory containing the currently running binary.
+     * Used only as part of the fallback configuration file lookup; exposed for unit testing.


No fallback or candidates. It's a known path or an error.

EDIT: took a look at communication, here it's a default constructor or given path, but the overall approach is different - RuntimeConfiguration is created first, then provided to Initialize function. This is incompatible with persistency approach, but as mentioned in other comment - maybe other places in persistency component need a redesign too.

arkjedrz · 2026-10-27T11:52:04Z

+     * Determine the directory containing the currently running binary.
+     * Used only as part of the fallback configuration file lookup; exposed for unit testing.
+     */
+    std::optional<std::string> running_binary_directory() const;


Do not pollute public API. I disagree this is needed, but if it is - move to anonymous namespace, and recreate in unit tests.

Ahmed-Elsaka-JC requested review from PandaeDo, antonkri, umaucher and vinodreddy-g as code owners October 26, 2026 14:05

Ahmed-Elsaka-JC deployed to workflow-approval October 26, 2026 14:05 — with GitHub Actions Active

Ahmed-Elsaka-JC deployed to workflow-approval October 26, 2026 14:06 — with GitHub Actions Active

Ahmed-Elsaka-JC force-pushed the afe_config_by_file_per branch from a358d13 to 417bc2b Compare October 27, 2026 10:52

Ahmed-Elsaka-JC deployed to workflow-approval October 27, 2026 10:52 — with GitHub Actions Active

implement configuration by file.

308a7d9

Ahmed-Elsaka-JC force-pushed the afe_config_by_file_per branch from 417bc2b to 308a7d9 Compare October 27, 2026 11:33

Ahmed-Elsaka-JC had a problem deploying to workflow-approval October 27, 2026 11:33 — with GitHub Actions Error

arkjedrz requested changes Oct 27, 2026

View reviewed changes

fix lint findings

4ad857d

Ahmed-Elsaka-JC deployed to workflow-approval October 27, 2026 12:08 — with GitHub Actions Active

Conversation

Ahmed-Elsaka-JC commented Oct 26, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

arkjedrz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This branch was successfully deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Oct 26, 2026 •

edited

Loading