-
Notifications
You must be signed in to change notification settings - Fork 69
Validate scale numeric options before actions run #742
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { Command } from 'commander'; | ||
| import { Command, InvalidArgumentError } from 'commander'; | ||
| import kleur from 'kleur'; | ||
| import { readFileSync, writeFileSync, existsSync, mkdirSync } from 'node:fs'; | ||
| import { homedir } from 'node:os'; | ||
|
|
@@ -12,6 +12,30 @@ import { deployCmd } from './deploy.js'; | |
| const CREDS_FILE = join(homedir(), '.sh1pt', 'credentials.json'); | ||
| const ROLLOUTS_FILE = join(homedir(), '.sh1pt', 'rollouts.json'); | ||
|
|
||
| export function parsePositiveSafeInteger(value: string): number { | ||
| const parsed = Number(value); | ||
| if (!Number.isSafeInteger(parsed) || parsed < 1) { | ||
| throw new InvalidArgumentError('must be a positive safe integer'); | ||
| } | ||
| return parsed; | ||
| } | ||
|
|
||
| export function parseNonNegativeSafeInteger(value: string): number { | ||
| const parsed = Number(value); | ||
| if (!Number.isSafeInteger(parsed) || parsed < 0) { | ||
| throw new InvalidArgumentError('must be a non-negative safe integer'); | ||
| } | ||
| return parsed; | ||
| } | ||
|
|
||
| export function parsePositiveFiniteNumber(value: string): number { | ||
| const parsed = Number(value); | ||
| if (!Number.isFinite(parsed) || parsed <= 0) { | ||
| throw new InvalidArgumentError('must be a positive finite number'); | ||
| } | ||
| return parsed; | ||
| } | ||
|
|
||
| export interface FleetEntry { | ||
| id: string; | ||
| provider: string; | ||
|
|
@@ -165,9 +189,9 @@ scaleCmd.addCommand(deployCmd); | |
| scaleCmd | ||
| .command('up') | ||
| .description('Buy more instances of the current SKU (via sh1pt deploy under the hood)') | ||
| .option('--instances <n>', 'how many to add', Number, 1) | ||
| .option('--instances <n>', 'how many to add', parsePositiveSafeInteger, 1) | ||
| .option('--provider <id>', 'which cloud provider to add to (default: same as existing fleet, or first in pricing table)') | ||
| .option('--max-hourly-price <usd>', 'abort if the new instances would push above this total/hr', Number) | ||
| .option('--max-hourly-price <usd>', 'abort if the new instances would push above this total/hr', parsePositiveFiniteNumber) | ||
| .option('--dry-run', 'show the plan without modifying state') | ||
| .action((opts: { | ||
| instances: number; | ||
|
|
@@ -262,7 +286,7 @@ scaleCmd | |
| scaleCmd | ||
| .command('down') | ||
| .description('Tear down instances (cheapest / least-healthy first)') | ||
| .option('--instances <n>', 'number of instances to destroy', Number, 1) | ||
| .option('--instances <n>', 'number of instances to destroy', parsePositiveSafeInteger, 1) | ||
| .option('--provider <id>', 'only remove instances from this cloud provider') | ||
| .option('--dry-run', 'show the plan without modifying state') | ||
| .option('--json', 'machine-readable output') | ||
|
|
@@ -371,10 +395,10 @@ scaleCmd | |
| scaleCmd | ||
| .command('auto') | ||
| .description('Set auto-scale rules (sh1pt cloud polls metrics and runs scale up/down on your behalf)') | ||
| .option('--min <n>', 'minimum instances', Number, 1) | ||
| .option('--max <n>', 'maximum instances', Number, 10) | ||
| .option('--target-cpu <percent>', 'target CPU utilization to maintain', Number, 70) | ||
| .option('--cooldown <seconds>', 'minimum time between scale events', Number, 300) | ||
| .option('--min <n>', 'minimum instances', parseNonNegativeSafeInteger, 1) | ||
| .option('--max <n>', 'maximum instances', parsePositiveSafeInteger, 10) | ||
| .option('--target-cpu <percent>', 'target CPU utilization to maintain', parsePositiveSafeInteger, 70) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| .option('--cooldown <seconds>', 'minimum time between scale events', parsePositiveSafeInteger, 300) | ||
| .option('--status', 'show current auto-scale rules') | ||
| .option('--dry-run', 'show the rules without saving') | ||
| .option('--json', 'machine-readable output') | ||
|
|
@@ -497,7 +521,7 @@ scaleCmd | |
| .description('Wire round-robin DNS so traffic spreads across the fleet') | ||
| .requiredOption('--provider <id>', 'dns-porkbun | dns-cloudflare') | ||
| .requiredOption('--domain <fqdn>', 'e.g. api.example.com') | ||
| .option('--ttl <seconds>', 'TTL for DNS records', Number, 60) | ||
| .option('--ttl <seconds>', 'TTL for DNS records', parsePositiveSafeInteger, 60) | ||
| .option('--proxied', 'cloudflare only — route through the CF edge (orange cloud)') | ||
| .option('--dry-run', 'show the DNS records that would be created/updated') | ||
| .option('--json', 'machine-readable output') | ||
|
|
@@ -609,7 +633,7 @@ scaleCmd | |
| .description('Stage a new version across the fleet (canary / blue-green / rolling)') | ||
| .requiredOption('--version <id>', 'version identifier to deploy (e.g. v2.1.0)') | ||
| .option('--strategy <kind>', 'canary | blue-green | rolling', 'canary') | ||
| .option('--percent <n>', 'canary only — start at N% of traffic', Number, 5) | ||
| .option('--percent <n>', 'canary only — start at N% of traffic', parsePositiveSafeInteger, 5) | ||
| .option('--dry-run', 'show the plan without modifying state') | ||
| .option('--status', 'show active rollouts and their state') | ||
| .option('--rollback <id>', 'roll back a previously completed rollout by ID') | ||
|
|
||
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.
parseNonNegativeSafeIntegerThe
parsePositiveSafeIntegersuite explicitly tests'9007199254740992'(2^53, the first unsafe integer), but theparseNonNegativeSafeIntegersuite omits that boundary case. Adding it would ensure the upper bound ofNumber.isSafeIntegeris uniformly exercised across all three parsers.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!