Skip to content

S3UTILS-227: replace util.parseArgs with manual argv parsing #381

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.

Sign up for GitHub

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

Merged
bert-e merged 2 commits into from
Apr 1, 2026
Merged

S3UTILS-227: replace util.parseArgs with manual argv parsing #381

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e177a1
S3UTILS-227: replace util.parseArgs with manual argv parsing
nicolas2bert Mar 31, 2026
ec54481
S3UTILS-227: guard --iam-port against missing value
nicolas2bert Mar 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 25 additions & 13 deletions replicationAudit/fix-missing-replication-permissions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
const fs = require('fs');
const http = require('http');
const https = require('https');
const { parseArgs } = require('util');
const { Client: VaultClient } = require('vaultclient');
const AWS = require('aws-sdk');

Expand All @@ -49,17 +48,30 @@ function log(message) {
}

// ===========================================================================
// Argument parsing (Node.js 22+ built-in util.parseArgs)
// Argument parsing (manual process.argv for Node 16+ compatibility)
// ===========================================================================
function getConfig() {
const { values, positionals } = parseArgs({
allowPositionals: true,
options: {
'iam-port': { type: 'string', default: '8600' },
'https': { type: 'boolean', default: false },
'dry-run': { type: 'boolean', default: false },
},
});
const args = process.argv.slice(2);
Copy link
Copy Markdown
Contributor

@anurag4DSB anurag4DSB Mar 31, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nicolas2bert nicolas2bert Apr 1, 2026

Choose a reason for hiding this comment

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

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.


const positionals = [];
let iamPort = 8600;
let useHttps = false;
let dryRun = false;

for (let i = 0; i < args.length; i++) {
if (args[i] === '--iam-port') {
if (i + 1 >= args.length) {
throw new Error('Missing value for --iam-port');
}
iamPort = parseInt(args[++i], 10);
Copy link
Copy Markdown

@claude claude bot Mar 31, 2026

Choose a reason for hiding this comment

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

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

} else if (args[i] === '--https') {
useHttps = true;
} else if (args[i] === '--dry-run') {
dryRun = true;
} else {
positionals.push(args[i]);
}
}

if (positionals.length < 3) {
log('Usage: node fix-missing-replication-permissions.js'
Expand All @@ -72,11 +84,11 @@ function getConfig() {
return {
inputFile,
vaultHost,
iamPort: parseInt(values['iam-port'], 10),
iamPort,
adminConfig,
useHttps: values.https,
useHttps,
outputFile: outputFile || 'replication-fix-results.json',
dryRun: values['dry-run'],
dryRun,
};
}

Expand Down
Loading