erpl-rev setup / doctor: close the gap between uvx erpl-rev and a working system - #82
Merged
Conversation
`uvx erpl-rev` puts a running binary on a machine in seconds; getting the
SAP side ready was still an afternoon spread across four documents -- a
type-T destination, a function group, eight `Z_DUCKDB_*` modules, fourteen
ABAP objects, a reginfo line and possibly a comm user.
doctor diagnoses and never writes.
setup diagnoses, shows the change set, asks, does it, and then proves
a round trip before claiming anything.
The ABAP travels inside the binary (cmake/embed_abap.cmake, 14 production
objects; no test drivers or fixtures) because setup has no checkout to read
from. `sapgw00` and the PROGRAM_ID are templated at deploy time -- the
shipped literal is wrong on every instance that is not 00, and it fails
later as an empty SYSTEM_FAILURE that names nothing.
Two things genuinely cannot be done from a client: the gateway reginfo
allow-list and the gw/acl_mode profile parameters. Rather than pretend,
setup writes erpl-rev-basis-handout.md filled in for the system it just
diagnosed.
Deliberate decisions:
- Every write is preceded by a terminal confirmation or an explicit --yes.
`--non-interactive` means "do not prompt", not "yes, change my ERP".
- Success is a proved round trip, never "the objects exist". Both classruns
are judged by their output, not their exit code: ZCL_ERPL_REV_SETUP
swallows RFC_MODIFY_TCPIP_DESTINATION behind EXCEPTIONS OTHERS = 9 and
ZCL_ERPL_REV_MKFM catches cx_root, so both exit 0 having achieved nothing.
The destination is verified out of RFCDES (registration mode, PROGRAM_ID
and gateway service all checked) and the modules out of TFDIR (fmode=R).
- Unknown flags are now a hard error. A server silently running with a
typo'd flag is worse than one that refuses to start.
- The password reaches erpl-adt through the environment, never argv.
Verified against A4H: bare-system deploy, dry-run writes nothing, a green
run ending in a proved round trip, a second run that reports "nothing to
do" and changes nothing, and the 13-stage e2e suite still passing
afterwards. 12 unit tests cover the planner, the handout and the classrun
output parsing.
Also corrects docs/INSTALL.md, which claimed the setup classrun verified
the FMs and printed the reginfo line. It did neither.
Reviewed by codex; the consent gate, the classrun verification, the
Windows quoting refusal and the save-settings prompt come from that pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The classrun parsers decide whether a customer's ERP was configured
correctly, and the first versions decided it from loose substrings. Every
input below was accepted before and is now rejected, each with a test:
setup subrc=0 opts=[H=%%RFCSERVER%%,g=sapgw00,N=ERPL_REV2,]
passed when the expected PROGRAM_ID was ERPL_REV.
setup subrc=0 opts=[H=%%RFCSERVER%%,g=sapgw00,N=ERPL_REV,]
passed when the expected gateway service was sapgw0.
setup subrc=0 opts=[]
debug: H=%%RFCSERVER%% N=ERPL_REV g=sapgw00
passed on a destination with no options at all, by reading the
values out of an unrelated line.
Z_DUCKDB_QUERY tfdir subrc=0 fmode=R (eight times)
passed as "all eight modules present" while seven were missing.
Now: the decision is read from the one line that reported it, RFCDES
options are compared as exact key=value tokens, and every TFDIR line is
bound to the module name it is claimed for. A failure names which modules
are absent instead of a bare count.
Two config-file defects from the same review:
- A password stored once by --save-password survived every later save that
did not ask for it, because the map was seeded from the existing file.
Consent is now per run: the key is dropped unless it was given again.
- WriteConfig truncated the real file under the umask and chmod'ed after
closing, so a file that may hold a password was briefly world-readable,
and a crash mid-write left the config truncated. It now writes a 0600
temp file and renames over the target.
Also: --print-runbook claimed "everything else has already been done"
when nothing had run yet, which would send a Basis team looking for
objects that are not there.
Verified live: interactive TTY flow (prompts, no echo, offer to save,
0600, no password written), stale-password drop, --save-password storing
and warning, --print-runbook, doctor --json, wrong credentials (401
diagnosed, dependent checks skipped not cascaded), unreachable gateway,
unknown verb and flag. Full suite 16567 assertions; e2e 13/13.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three platform mistakes, all mine, all caught by CI: - `_isatty` needs <io.h>; `addrinfo`/`getaddrinfo` need <ws2tcpip.h>. winsock2.h has to precede windows.h or the winsock 1.1 declarations collide with it. - No WSAStartup anywhere in the process. This one would have built cleanly and then lied: getaddrinfo fails with WSANOTINITIALISED, so doctor would report a perfectly healthy gateway as unreachable and send people to look at firewalls. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Two new verbs.
doctordiagnoses the SAP side and never writes to it;setupdiagnoses, prints the change set, asks, executes, and then proves a round trip
before claiming anything.
Why
Installing was already one command. Getting SAP ready was an afternoon spread
across
README.md,docs/INSTALL.md,docs/security.mdanddocs/enable-rfc-registration.md: a type-T destination, a function group, eightZ_DUCKDB_*modules, fourteen ABAP objects, areginfoline, a comm user.scripts/deploy-abap.shalready encoded most of the sequence — but only as bashneeding a git checkout.
How
cmake/embed_abap.cmake, 14 productionobjects — no test drivers, no fixtures).
setuphas no checkout to read from.sapgw00and the PROGRAM_ID are templated at deploy time. The shippedliteral is wrong on every instance that is not
00, and it fails much later asan empty
SYSTEM_FAILUREthat names nothing.reginfoallow-list andthe
gw/acl_modeprofile parameters are an OS file and an instance restart.setupwriteserpl-rev-basis-handout.md, filled in for the system it justdiagnosed, with nothing left to compose.
Decisions worth arguing about
--yes.--non-interactivemeans "do not prompt" — not "yes, change my ERP system".are judged by their output, not their exit code:
ZCL_ERPL_REV_SETUPswallowsRFC_MODIFY_TCPIP_DESTINATIONbehindEXCEPTIONS OTHERS = 9andZCL_ERPL_REV_MKFMcatchescx_root, so both exit 0 having achieved nothing.The destination is verified out of
RFCDES(registration mode, PROGRAM_ID andgateway service each checked) and the modules out of
TFDIR(fmode=R).a typo'd flag is worse than one that refuses to start. This is an intentional
behaviour change; the no-verb
servepath is unaffected.erpl-adtthrough the environment, never argv.Verification
Live against A4H:
--dry-rundoctornames what is missing and skips the round trip with a reason, exit 1;setupdeploys all 14 in dependency order, creates the function group, both classruns verified, round trip proved, exit 0setupalone is sufficient to provision a wiped systemnothing — the SAP side is already set up., exit 0, no writes0600, no password stored--save-passworddoctorwrong passworddoctorunreachable gateway--print-runbookdoctor --json_isatty/getaddrinfoheaders and a missingWSAStartupfixed — the latter would have built cleanly and then reported a healthy gateway as unreachableUnit: 16567 assertions, 14 of them over the planner, the handout and the
classrun parsers.
Review
Two codex passes (gpt-5.5, medium), the second one specifically against the
result of the first — so the fixes were reviewed, not just the original.
Pass 1 (five findings, all addressed): the consent gate, verifying classruns by
output rather than exit code, a Windows quoting path that could silently mangle
%,--save-password/WriteConfigadvertised but dead, and a doc point aboutwhat
--dry-runguarantees.Pass 2 found four real defects in the fixes, each now rejected with a test:
N=ERPL_REV2satisfied an expectedERPL_REV;g=sapgw00satisfied anexpected
sapgw0— prefix matches on what should be exact tokens.opts=[]plus a diagnostic echo passed on a destination with no options.Z_DUCKDB_QUERY tfdir subrc=0 fmode=Rlines passed as "alleight modules present" while seven were missing.
--save-passwordsurvived every later save that didnot ask for it, and
WriteConfigchmod'ed to0600only after writing thecontent under the umask.
One bug the dry run itself caught: checking a single representative object made a
system deployed before
ZCL_ERPL_REV_DIAGexisted look complete, sosetupplanned to prove a round trip with a class it had never deployed. Regression test
included.
Also corrects
docs/INSTALL.md, which claimed the setup classrun "verifies theZERPL_REVFMs exist; prints the exactreginfoline to add". It did neither.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.