S3UTILS-227: replace util.parseArgs with manual argv parsing#381
S3UTILS-227: replace util.parseArgs with manual argv parsing#381bert-e merged 2 commits intodevelopment/1.17from
Conversation
Problem: fix-missing-replication-permissions.js uses util.parseArgs which requires Node 16.17+. The vault container ships Node 16.13.2 on all S3C versions up to 9.5.0.x, causing "parseArgs is not a function" at runtime. Fix: Replace util.parseArgs with manual process.argv parsing. No other dependency changes. There is no risk with other dependencies: vaultclient, aws-sdk v2, and process.argv all work on Node 16.13.2+. Testing: Existing functional tests in fixMissingReplicationPermissions.js run the script as a child process through its CLI interface (positionals, --iam-port, --https, --dry-run). Since the CLI contract is unchanged, passing tests confirm the new parsing logic is equivalent.
Hello nicolas2bert,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| for (let i = 0; i < args.length; i++) { | ||
| if (args[i] === '--iam-port') { | ||
| iamPort = parseInt(args[++i], 10); |
There was a problem hiding this comment.
If --iam-port is the last argument (no value follows), args[++i] is undefined and parseInt(undefined, 10) silently returns NaN. The original parseArgs would have thrown. Consider guarding against this with a bounds check before incrementing i.
— Claude Code
|
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Add bounds check before consuming the next argument so that passing --iam-port as the last argument throws instead of silently setting the port to NaN.
|
LGTM |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## development/1.17 #381 +/- ##
====================================================
- Coverage 43.57% 43.48% -0.10%
====================================================
Files 87 87
Lines 6208 6221 +13
Branches 1299 1303 +4
====================================================
Hits 2705 2705
- Misses 3456 3469 +13
Partials 47 47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| 'dry-run': { type: 'boolean', default: false }, | ||
| }, | ||
| }); | ||
| const args = process.argv.slice(2); |
There was a problem hiding this comment.
My thoughts but consolidated using claude LLM:
The current implementation works, but the parsing logic is tightly coupled, order-dependent, and will become harder to maintain as flags grow.
A few specific concerns:
- We’re manually mutating the loop index (i++) to consume values, which makes the control flow harder to reason about and easier to break when adding new flags.
- Validation is inconsistent, parseInt is used without checking for NaN, which can lead to silent misconfiguration.
- The if/else chain does not scale; every new flag increases cognitive load and duplication.
Parsing, validation, and state mutation are all interleaved, making this difficult to test in isolation.
If we want to solve an already solved problem(see last paragraph), I’d recommend moving toward a table-driven approach where each flag maps to a handler. That separates concerns and makes the system extensible without modifying the core loop. At a minimum, a cas-switch-based structure with explicit validation would improve readability and safety.
More broadly, this is reimplementing CLI parsing logic that libraries like minimist or commander already handle robustly (including defaults, type coercion, aliases, and help text). If this script is expected to evolve beyond a couple of flags, introducing a lightweight dependency would reduce long-term complexity and edge cases.
There was a problem hiding this comment.
This ephemeral script (will be deleted o nce we update replication permissions for all clients using CRR cf PR description) is meant to be copied into older vault containers where only a minimal set of dependencies exist.
Using minimist would introduce an additional dependency, and we would need to ensure it behaves consistently across different Vault versions.
for 3 flags, the manual approach is arguably fine. The commit message on HEAD (ec54481) even says "replace util.parseArgs with manual argv parsing" -> so this was a deliberate choice for compatibility.
|
@bert-e approve |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue S3UTILS-227. Goodbye nicolas2bert. The following options are set: approve |
4 participants
Design note:
fix-missing-replication-permissions.jsis a standalone script meant to be copied into the vault container of older S3C versions. It does not run inside the s3utils container. It has to be compatible with the Node and npm packages available in the target vault container.This "copy a self-contained script into an older container" approach is not easy to implement, test, or maintain, and is not a good practice in general.
We accepted this trade-off after discussing with Product and CS to allow fast integration on customer environments.
This tool should not be needed once we update replication permissions for all clients using CRR.
Problem:
fix-missing-replication-permissions.jsusesutil.parseArgswhich requires Node 16.17+. The vault container ships Node 16.13.2 on all S3C versions up to 9.5.0.x, causing "parseArgs is not a function" at runtime.Fix:
Replace
util.parseArgswith manualprocess.argvparsing. No other dependency changes. There is no risk with other dependencies: vaultclient, aws-sdk v2, and process.argv all work on Node 16.13.2+.Testing:
Existing functional tests in
fixMissingReplicationPermissions.jsrun the script as a child process through its CLI interface (positionals, --iam-port, --https, --dry-run). Since the CLI contract is unchanged, passing tests confirm the new parsing logic is equivalent.