mirror of
https://github.com/Azure/k8s-deploy.git
synced 2026-06-28 07:49:27 +08:00
Add timeout to the rollout status (#425)
* Added timeout to the rollout status and tests for it * Fixed integration test errors * Fix for blue green integration test * Probable fix for integration errors * No jobs run error fixed * Changed timeout to file level constant * Added parsing logic for timeout * Made tests more concise * implemented timeout validation check in an extracted utils mod * Changed function name to parseDuration * Removed timeout parameter from getResource --------- Co-authored-by: David Gamero <david340804@gmail.com> Co-authored-by: Suneha Bose <123775811+bosesuneha@users.noreply.github.com>
This commit is contained in:
benjamin
@@ -0,0 +1,166 @@
|
||||
import {parseDuration} from './durationUtils'
|
||||
import * as core from '@actions/core'
|
||||
|
||||
// Mock core.debug
|
||||
jest.mock('@actions/core')
|
||||
const mockCore = core as jest.Mocked<typeof core>
|
||||
|
||||
// Test data constants
|
||||
const VALID_TIMEOUTS = {
|
||||
withUnits: ['5s', '10m', '1h', '500ms'],
|
||||
decimals: ['0.5s', '1.25m', '2.5h'],
|
||||
caseInsensitive: ['5S', '10M', '1H'],
|
||||
expectedLowercase: ['5s', '10m', '1h'],
|
||||
bareNumbers: ['5', '15', '120'],
|
||||
expectedWithMinutes: ['5m', '15m', '120m'],
|
||||
whitespace: [' 10s', '1m ', '\t2h\n'],
|
||||
expectedTrimmed: ['10s', '1m', '2h'],
|
||||
rangeValid: ['1ms', '999ms', '0.5s', '1439m', '23.999h'],
|
||||
edgeCases: ['0.001s', '0.0167m', '24h']
|
||||
}
|
||||
|
||||
const INVALID_TIMEOUTS = {
|
||||
badFormats: ['', 'abc', '30x', '30 s', '30sm'],
|
||||
negative: ['-5m', '-1s', '-0.5h'],
|
||||
zero: ['0s', '0m', '0h', '0ms'],
|
||||
belowMin: ['0.0001s', '0.00001ms'],
|
||||
aboveMax: ['25h', '1441m', '86401s']
|
||||
}
|
||||
|
||||
const ERROR_MESSAGES = {
|
||||
invalidFormat: (input: string) =>
|
||||
`Invalid duration format: "${input}". Use: number + unit (30s, 5m, 1h) or just number (assumes minutes)`,
|
||||
notPositive: (input: string) => `Duration must be positive: "${input}"`,
|
||||
outOfRange: (input: string) =>
|
||||
`Duration out of range (1ms to 24h): "${input}"`
|
||||
}
|
||||
|
||||
// Helper functions
|
||||
const expectValidTimeout = (input: string, expected: string) => {
|
||||
expect(parseDuration(input)).toBe(expected)
|
||||
}
|
||||
|
||||
const expectInvalidTimeout = (input: string, expectedError: string) => {
|
||||
expect(() => parseDuration(input)).toThrow(expectedError)
|
||||
}
|
||||
|
||||
describe('validateTimeoutDuration', () => {
|
||||
beforeEach(() => {
|
||||
jest.clearAllMocks()
|
||||
})
|
||||
|
||||
describe('valid timeout formats', () => {
|
||||
const validCases: Array<[string, string, string]> = [
|
||||
...VALID_TIMEOUTS.withUnits.map((v): [string, string, string] => [
|
||||
v,
|
||||
v,
|
||||
'accepts number with valid units'
|
||||
]),
|
||||
...VALID_TIMEOUTS.decimals.map((v): [string, string, string] => [
|
||||
v,
|
||||
v,
|
||||
'accepts decimal number with units'
|
||||
]),
|
||||
...VALID_TIMEOUTS.caseInsensitive.map(
|
||||
(v, i): [string, string, string] => [
|
||||
v,
|
||||
VALID_TIMEOUTS.expectedLowercase[i],
|
||||
'handles case-insensitive units'
|
||||
]
|
||||
),
|
||||
...VALID_TIMEOUTS.bareNumbers.map((v, i): [string, string, string] => [
|
||||
v,
|
||||
VALID_TIMEOUTS.expectedWithMinutes[i],
|
||||
'assumes minutes for bare numbers'
|
||||
]),
|
||||
...VALID_TIMEOUTS.whitespace.map((v, i): [string, string, string] => [
|
||||
v,
|
||||
VALID_TIMEOUTS.expectedTrimmed[i],
|
||||
'trims whitespace'
|
||||
])
|
||||
]
|
||||
|
||||
test.each(validCases)('%s → %s (%s)', (input, expected, description) => {
|
||||
expectValidTimeout(input, expected)
|
||||
})
|
||||
|
||||
test('logs assumption for bare numbers only', () => {
|
||||
parseDuration('5')
|
||||
expect(mockCore.debug).toHaveBeenCalledWith(
|
||||
'No unit specified for timeout "5", assuming minutes'
|
||||
)
|
||||
|
||||
jest.clearAllMocks()
|
||||
|
||||
parseDuration('30s')
|
||||
expect(mockCore.debug).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
describe('invalid timeout formats', () => {
|
||||
const invalidCases: Array<[string, string]> = [
|
||||
...INVALID_TIMEOUTS.badFormats.map((t): [string, string] => [
|
||||
t,
|
||||
ERROR_MESSAGES.invalidFormat(t)
|
||||
]),
|
||||
...INVALID_TIMEOUTS.negative.map((t): [string, string] => [
|
||||
t,
|
||||
ERROR_MESSAGES.invalidFormat(t)
|
||||
]),
|
||||
...INVALID_TIMEOUTS.zero.map((t): [string, string] => [
|
||||
t,
|
||||
ERROR_MESSAGES.notPositive(t)
|
||||
])
|
||||
]
|
||||
|
||||
test.each(invalidCases)('rejects %s', (input, expectedError) => {
|
||||
expectInvalidTimeout(input, expectedError)
|
||||
})
|
||||
})
|
||||
|
||||
describe('range validation', () => {
|
||||
const rangeCases: Array<[string, string, boolean]> = [
|
||||
...VALID_TIMEOUTS.rangeValid.map((v): [string, string, boolean] => [
|
||||
v,
|
||||
v,
|
||||
true
|
||||
]),
|
||||
...INVALID_TIMEOUTS.belowMin.map((v): [string, string, boolean] => [
|
||||
v,
|
||||
ERROR_MESSAGES.outOfRange(v),
|
||||
false
|
||||
]),
|
||||
...INVALID_TIMEOUTS.aboveMax.map((v): [string, string, boolean] => [
|
||||
v,
|
||||
ERROR_MESSAGES.outOfRange(v),
|
||||
false
|
||||
]),
|
||||
...VALID_TIMEOUTS.edgeCases.map((v): [string, string, boolean] => [
|
||||
v,
|
||||
v,
|
||||
true
|
||||
])
|
||||
]
|
||||
|
||||
test.each(rangeCases)('%s is %s', (input, expected, isValid) => {
|
||||
if (isValid) {
|
||||
expectValidTimeout(input, expected)
|
||||
} else {
|
||||
expectInvalidTimeout(input, expected)
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('edge cases', () => {
|
||||
test.each([
|
||||
['0.001s', '0.001s'],
|
||||
['0.0167m', '0.0167m'],
|
||||
['23.999h', '23.999h'],
|
||||
['1439m', '1439m'],
|
||||
['5.0m', '5m'],
|
||||
['005s', '5s']
|
||||
])('parses and normalizes: %s → %s', (input, expected) => {
|
||||
expectValidTimeout(input, expected)
|
||||
})
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,38 @@
|
||||
import * as core from '@actions/core'
|
||||
|
||||
export function parseDuration(duration: string): string {
|
||||
const trimmed = duration.trim()
|
||||
|
||||
// Parse number and optional unit using regex
|
||||
const match = /^(\d+(?:\.\d+)?)(ms|s|m|h)?$/i.exec(trimmed)
|
||||
if (!match) {
|
||||
throw new Error(
|
||||
`Invalid duration format: "${duration}". Use: number + unit (30s, 5m, 1h) or just number (assumes minutes)`
|
||||
)
|
||||
}
|
||||
|
||||
const value = parseFloat(match[1])
|
||||
const unit = match[2]?.toLowerCase() || 'm'
|
||||
|
||||
if (value <= 0) {
|
||||
throw new Error(`Duration must be positive: "${duration}"`)
|
||||
}
|
||||
|
||||
// Calculate total seconds for validation
|
||||
const multipliers = {ms: 0.001, s: 1, m: 60, h: 3600}
|
||||
const totalSeconds = value * multipliers[unit as keyof typeof multipliers]
|
||||
|
||||
// Validate bounds (1ms to 24h)
|
||||
if (totalSeconds < 0.001 || totalSeconds > 86400) {
|
||||
throw new Error(`Duration out of range (1ms to 24h): "${duration}"`)
|
||||
}
|
||||
|
||||
// Log assumption for bare numbers (when no unit was provided)
|
||||
if (!duration.trim().match(/\d+(ms|s|m|h)$/i)) {
|
||||
core.debug(
|
||||
`No unit specified for timeout "${duration}", assuming minutes`
|
||||
)
|
||||
}
|
||||
|
||||
return `${value}${unit}`
|
||||
}
|
||||
@@ -49,4 +49,60 @@ describe('manifestStabilityUtils', () => {
|
||||
expect(checkRolloutStatusSpy).toHaveBeenCalled()
|
||||
expect(spy).toHaveReturned()
|
||||
})
|
||||
|
||||
it('should pass timeout to checkRolloutStatus when provided', async () => {
|
||||
const timeout = '300s'
|
||||
const checkRolloutStatusSpy = jest
|
||||
.spyOn(kc, 'checkRolloutStatus')
|
||||
.mockImplementation(() => {
|
||||
return new Promise<ExecOutput>((resolve, reject) => {
|
||||
resolve({
|
||||
exitCode: 0,
|
||||
stderr: '',
|
||||
stdout: ''
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
await manifestStabilityUtils.checkManifestStability(
|
||||
kc,
|
||||
resources,
|
||||
ResourceTypeManagedCluster,
|
||||
timeout
|
||||
)
|
||||
|
||||
expect(checkRolloutStatusSpy).toHaveBeenCalledWith(
|
||||
'deployment',
|
||||
'test',
|
||||
'default',
|
||||
timeout
|
||||
)
|
||||
})
|
||||
|
||||
it('should call checkRolloutStatus without timeout when not provided', async () => {
|
||||
const checkRolloutStatusSpy = jest
|
||||
.spyOn(kc, 'checkRolloutStatus')
|
||||
.mockImplementation(() => {
|
||||
return new Promise<ExecOutput>((resolve, reject) => {
|
||||
resolve({
|
||||
exitCode: 0,
|
||||
stderr: '',
|
||||
stdout: ''
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
await manifestStabilityUtils.checkManifestStability(
|
||||
kc,
|
||||
resources,
|
||||
ResourceTypeManagedCluster
|
||||
)
|
||||
|
||||
expect(checkRolloutStatusSpy).toHaveBeenCalledWith(
|
||||
'deployment',
|
||||
'test',
|
||||
'default',
|
||||
undefined
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -12,7 +12,8 @@ const POD = 'pod'
|
||||
export async function checkManifestStability(
|
||||
kubectl: Kubectl,
|
||||
resources: Resource[],
|
||||
resourceType: ClusterType
|
||||
resourceType: ClusterType,
|
||||
timeout?: string
|
||||
): Promise<void> {
|
||||
// Skip if resource type is microsoft.containerservice/fleets
|
||||
if (resourceType === ResourceTypeFleet) {
|
||||
@@ -32,7 +33,8 @@ export async function checkManifestStability(
|
||||
const result = await kubectl.checkRolloutStatus(
|
||||
resource.type,
|
||||
resource.name,
|
||||
resource.namespace
|
||||
resource.namespace,
|
||||
timeout
|
||||
)
|
||||
checkForErrors([result])
|
||||
} catch (ex) {
|
||||
|
||||
Reference in New Issue
Block a user