Skip to content
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

Use InputPropertyError for validations in the node group components #1592

Merged
merged 3 commits into from
Jan 24, 2025
Merged

Use InputPropertyError for validations in the node group components #1592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b11aa46
Use InputPropertyError for validations in the node group components
flostadler Jan 10, 2025
656a163
Lint
flostadler Jan 22, 2025
929ccdc
Correct error message
flostadler Jan 23, 2025
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
10 changes: 5 additions & 5 deletions nodejs/eks/nodes/ami.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,29 +46,29 @@ describe("toAmiType", () => {
});
describe("getOperatingSystem", () => {
test("should return the provided operating system if available", () => {
const operatingSystem = getOperatingSystem(undefined, OperatingSystem.AL2023, undefined);
const operatingSystem = getOperatingSystem(undefined, OperatingSystem.AL2023);
expect(operatingSystem).toBe(OperatingSystem.AL2023);
});

test("should return the default operating system if no AMI type or operating system is provided", () => {
const operatingSystem = getOperatingSystem(undefined, undefined, undefined);
const operatingSystem = getOperatingSystem(undefined, undefined);
expect(operatingSystem).toBe(DEFAULT_OS);
});

test("should resolve the operating system based on the provided AMI type", () => {
const operatingSystem = getOperatingSystem("AL2023_ARM_64_STANDARD", undefined, undefined);
const operatingSystem = getOperatingSystem("AL2023_ARM_64_STANDARD", undefined);
expect(operatingSystem).toBe(OperatingSystem.AL2023);
});

test("should throw an error for unknown AMI type", () => {
expect(() => {
getOperatingSystem("unknown-ami-type", undefined, undefined);
getOperatingSystem("unknown-ami-type", undefined);
}).toThrow("Cannot determine OS of unknown AMI type: unknown-ami-type");
});

test("should throw an error if the operating system does not match the AMI type", () => {
expect(() => {
getOperatingSystem("AL2023_ARM_64_STANDARD", OperatingSystem.Bottlerocket, undefined);
getOperatingSystem("AL2023_ARM_64_STANDARD", OperatingSystem.Bottlerocket);
}).toThrow(
"Operating system 'Bottlerocket' does not match the detected operating system 'AL2023' of AMI type 'AL2023_ARM_64_STANDARD'.",
);
Expand Down
17 changes: 8 additions & 9 deletions nodejs/eks/nodes/ami.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ export function toAmiType(str: string): AmiType | undefined {
export function getOperatingSystem(
amiType: string | undefined,
operatingSystem: OperatingSystem | undefined,
parent: pulumi.Resource | undefined,
): OperatingSystem {
if (operatingSystem && !amiType) {
return operatingSystem;
Expand All @@ -210,20 +209,20 @@ export function getOperatingSystem(

const resolvedAmiType = toAmiType(amiType);
if (!resolvedAmiType) {
throw new pulumi.ResourceError(
`Cannot determine OS of unknown AMI type: ${amiType}`,
parent,
corymhall marked this conversation as resolved.
Show resolved Hide resolved
);
throw new pulumi.InputPropertyError({
propertyPath: "amiType",
reason: `Cannot determine OS of unknown AMI type: ${amiType}`,
});
}

const resolvedOs = getAmiMetadata(resolvedAmiType).os;

// if users provided both OS and AMI type, we should check if they match
if (operatingSystem && operatingSystem !== resolvedOs) {
throw new pulumi.ResourceError(
`Operating system '${operatingSystem}' does not match the detected operating system '${resolvedOs}' of AMI type '${amiType}'.`,
parent,
);
throw new pulumi.InputPropertyError({
propertyPath: "operatingSystem",
reason: `Operating system '${operatingSystem}' does not match the detected operating system '${resolvedOs}' of AMI type '${amiType}'.`,
});
}

return resolvedOs;
Expand Down
93 changes: 30 additions & 63 deletions nodejs/eks/nodes/nodegroup.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,40 +307,40 @@ const nonGravitonInstances = [
describe("isGravitonInstance", () => {
gravitonInstances.forEach((instanceType) => {
test(`${instanceType} should return true for a Graviton instance`, () => {
expect(isGravitonInstance(instanceType, undefined)).toBe(true);
expect(isGravitonInstance(instanceType, "instanceTypes")).toBe(true);
});
});

nonGravitonInstances.forEach((instanceType) => {
test(`${instanceType} should return false for non-Graviton instance`, () => {
expect(isGravitonInstance(instanceType, undefined)).toBe(false);
expect(isGravitonInstance(instanceType, "instanceTypes")).toBe(false);
});
});
describe("getArchitecture", () => {
test("should return 'x86_64' when only x86_64 instances are provided", () => {
const instanceTypes = ["c5.large", "m5.large", "t3.large"];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("x86_64");
});

test("should return 'arm64' when only arm64 instances are provided", () => {
const instanceTypes = ["c6g.large", "m6g.large", "t4g.large"];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("arm64");
});

test("should throw an error when both x86_64 and arm64 instances are provided", () => {
const instanceTypes = ["c5.large", "c6g.large"];
expect(() => {
getArchitecture(instanceTypes, undefined);
}).toThrowError(
getArchitecture(instanceTypes, "instanceTypes");
}).toThrow(
"Cannot determine architecture of instance types. The provided instance types do not share a common architecture",
);
});

test("should return 'x86_64' when no instance types are provided", () => {
const instanceTypes: string[] = [];
const architecture = getArchitecture(instanceTypes, undefined);
const architecture = getArchitecture(instanceTypes, "instanceTypes");
expect(architecture).toBe("x86_64");
});
});
Expand Down Expand Up @@ -484,71 +484,52 @@ describe("resolveInstanceProfileName", function () {

test("no args, no c.nodeGroupOptions throws", async () => {
expect(() =>
resolveInstanceProfileName(
"nodegroup-name",
{},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError("an instanceProfile or instanceProfileName is required");
resolveInstanceProfileName({}, {
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>),
).toThrow("an instanceProfile or instanceProfileName is required");
});

test("both args.instanceProfile and args.instanceProfileName throws", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {});
const name = "nodegroup-name";
expect(() =>
resolveInstanceProfileName(
name,
{
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError(
`invalid args for node group ${name}, instanceProfile and instanceProfileName are mutually exclusive`,
);
).toThrow(`instanceProfile and instanceProfileName are mutually exclusive`);
});

test("both c.nodeGroupOptions.instanceProfileName and c.nodeGroupOptions.instanceProfile throws", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {});
const name = "nodegroup-name";
expect(() =>
resolveInstanceProfileName(
name,
{},
{
nodeGroupOptions: {
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
),
).toThrowError(
`invalid args for node group ${name}, instanceProfile and instanceProfileName are mutually exclusive`,
);
resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfile: instanceProfile,
instanceProfileName: "instanceProfileName",
},
} as pulumi.UnwrappedObject<CoreData>),
).toThrow(`instanceProfile and instanceProfileName are mutually exclusive`);
});

test("args.instanceProfile returns passed instanceProfile", async () => {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {
name: "passedInstanceProfile",
});
const nodeGroupName = "nodegroup-name";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{
instanceProfile: instanceProfile,
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const expected = await promisify(instanceProfile.name);
const recieved = await promisify(resolvedInstanceProfileName);
Expand All @@ -559,53 +540,39 @@ describe("resolveInstanceProfileName", function () {
const instanceProfile = new aws.iam.InstanceProfile("instanceProfile", {
name: "passedInstanceProfile",
});
const nodeGroupName = "nodegroup-name";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{},
{
nodeGroupOptions: {
instanceProfile: instanceProfile,
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const resolvedInstanceProfileName = resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfile: instanceProfile,
},
} as pulumi.UnwrappedObject<CoreData>);
const expected = await promisify(instanceProfile.name);
const recieved = await promisify(resolvedInstanceProfileName);
expect(recieved).toEqual(expected);
});

test("args.instanceProfileName returns passed InstanceProfile", async () => {
const nodeGroupName = "nodegroup-name";
const nodeGroupName = "-name";
const existingInstanceProfileName = "existingInstanceProfileName";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{
instanceProfileName: existingInstanceProfileName,
},
{
nodeGroupOptions: {},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const expected = existingInstanceProfileName;
const received = await promisify(resolvedInstanceProfileName);
expect(received).toEqual(expected);
});

test("nodeGroupOptions.instanceProfileName returns existing InstanceProfile", async () => {
const nodeGroupName = "nodegroup-name";
const existingInstanceProfileName = "existingInstanceProfileName";
const resolvedInstanceProfileName = resolveInstanceProfileName(
nodeGroupName,
{},
{
nodeGroupOptions: {
instanceProfileName: existingInstanceProfileName,
},
} as pulumi.UnwrappedObject<CoreData>,
undefined as any,
);
const resolvedInstanceProfileName = resolveInstanceProfileName({}, {
nodeGroupOptions: {
instanceProfileName: existingInstanceProfileName,
},
} as pulumi.UnwrappedObject<CoreData>);
const expected = existingInstanceProfileName;
const received = await promisify(resolvedInstanceProfileName);
expect(received).toEqual(expected);
Expand Down
Loading
Loading