-
-
Notifications
You must be signed in to change notification settings - Fork 4
Add a --configuration option to the index binary
#628
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
Conversation
Signed-off-by: Juan Cruz Viotti <jv@jviotti.com>
🤖 Augment PR SummarySummary: Adds a dedicated Changes:
Technical Notes: Configuration output is prettified to stdout and the command exits early after reading the configuration JSON. 🤖 Was this summary useful? React with 👍 or 👎 |
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.
| if (app.contains("verbose")) { | ||
| sourcemeta::core::prettify(raw_configuration, std::cerr); | ||
| std::cerr << "\n"; | ||
| if (app.contains("configuration")) { |
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.
--configuration returns before the --url override block later in index_main, so invocations like ... --url <...> --configuration will print the non-overridden URL (and other derived values) rather than the effective configuration; the Makefile now calls this combination in sandbox-index. Is that the intended behavior for the new option?
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| sourcemeta::core::prettify(raw_configuration, std::cerr); | ||
| std::cerr << "\n"; | ||
| if (app.contains("configuration")) { | ||
| sourcemeta::core::prettify(raw_configuration, std::cout); |
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.
In --configuration mode the JSON is written to stdout, but the program also prints the version banner to stdout at startup, so the overall stdout stream isn’t valid JSON (the new tests work around this via tail -n +2). If --configuration is meant for scripting/pipe use, this mixed output could be surprising.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
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.
2 issues found across 9 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/index/index.cc">
<violation number="1" location="src/index/index.cc:105">
P2: `--configuration` should be a read-only operation, but it still constructs `Output` before returning, which creates/scans the output directory. Consider moving the configuration-print/exit path before `Output` initialization so it doesn’t touch the filesystem when only dumping the configuration.</violation>
</file>
<file name="test/cli/index/enterprise/sourcemeta-std.sh">
<violation number="1" location="test/cli/index/enterprise/sourcemeta-std.sh:17">
P2: The pipeline only checks `tail`'s exit status, so a failure in the index command can be masked. Run the command separately (or capture its output) so `errexit` stops the test on failure.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (app.contains("verbose")) { | ||
| sourcemeta::core::prettify(raw_configuration, std::cerr); | ||
| std::cerr << "\n"; | ||
| if (app.contains("configuration")) { |
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.
P2: --configuration should be a read-only operation, but it still constructs Output before returning, which creates/scans the output directory. Consider moving the configuration-print/exit path before Output initialization so it doesn’t touch the filesystem when only dumping the configuration.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/index/index.cc, line 105:
<comment>`--configuration` should be a read-only operation, but it still constructs `Output` before returning, which creates/scans the output directory. Consider moving the configuration-print/exit path before `Output` initialization so it doesn’t touch the filesystem when only dumping the configuration.</comment>
<file context>
@@ -102,9 +102,10 @@ static auto index_main(const std::string_view &program,
- if (app.contains("verbose")) {
- sourcemeta::core::prettify(raw_configuration, std::cerr);
- std::cerr << "\n";
+ if (app.contains("configuration")) {
+ sourcemeta::core::prettify(raw_configuration, std::cout);
+ std::cout << "\n";
</file context>
| } | ||
| EOF | ||
|
|
||
| "$1" "$TMP/one.json" "$TMP/output" --configuration | tail -n +2 > "$TMP/output.txt" |
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.
P2: The pipeline only checks tail's exit status, so a failure in the index command can be masked. Run the command separately (or capture its output) so errexit stops the test on failure.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/cli/index/enterprise/sourcemeta-std.sh, line 17:
<comment>The pipeline only checks `tail`'s exit status, so a failure in the index command can be masked. Run the command separately (or capture its output) so `errexit` stops the test on failure.</comment>
<file context>
@@ -0,0 +1,54 @@
+}
+EOF
+
+"$1" "$TMP/one.json" "$TMP/output" --configuration | tail -n +2 > "$TMP/output.txt"
+
+cat << EOF > "$TMP/expected.txt"
</file context>
Signed-off-by: Juan Cruz Viotti jv@jviotti.com