Skip to content
This repository has been archived by the owner on Jun 7, 2023. It is now read-only.

Fix launch template optional API params #3

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

markpitchless
Copy link

@markpitchless markpitchless commented Oct 13, 2021

When using a launch template many of the params in the EC2 launch API
call are optional, but fail if you given an empty value, they need to
not be present. e.g. ImageID='' will fail. Therefore we only add them to
params if set.

The higher level validation on config handles start vs stop required
inputs.

fac/dev-platform#539

When using a launch template many of the params in the EC2 launch API
call are optional, but fail if you given an empty value, they need to
not be presemt. e.g. ImageID='' will fail. Therefore we only add them to
params if set.

The higher level validation on config handles start vs stop required
inputs.
MinCount: config.input.runnerCount,
MaxCount: config.input.runnerCount,
UserData: Buffer.from(userData.join('\n')).toString('base64'),
SubnetId: config.input.subnetId,
SecurityGroupIds: [config.input.securityGroupId],
IamInstanceProfile: { Name: config.input.iamRoleName },
TagSpecifications: config.tagSpecifications,
};

Choose a reason for hiding this comment

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

I might be being dumb, but where do you pass in a LaunchTemplate?

Copy link
Author

Choose a reason for hiding this comment

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

It came from the upstream PR we merged, so not in this diff, but just below here on line 52. Click the little expandy button.

Copy link
Author

Choose a reason for hiding this comment

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

This PR is just a fix for the upstream one, which didn't quite work. machulav#70

Choose a reason for hiding this comment

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

Ah! 💡 Thanks for the context 👌

@@ -55283,7 +55302,9 @@ class Config {
}

if (this.input.mode === 'start') {
if (!this.input.ec2ImageId || !this.input.ec2InstanceType || !this.input.subnetId || !this.input.securityGroupId) {
const isSet = param => param;
const params = [this.input.ec2ImageId, this.input.ec2InstanceType, this.input.subnetId, this.input.securityGroupId];
Copy link
Member

Choose a reason for hiding this comment

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

I was a bit confused about where this line came from: it's from #2, and appears here because the dist got rebuilt.

@markpitchless markpitchless merged commit 25633d1 into main Oct 13, 2021
@markpitchless markpitchless deleted the fix-launch-template-optional-params branch October 13, 2021 15:36
}

// when using a launch template any or all of these are optional
if (config.input.ec2ImageId) {
Copy link
Member

Choose a reason for hiding this comment

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

this could probably be done with an array of property names and dynamically checking config.input for them & adding them to params. But it would look like:

const passThrough = ['ec2ImageId', 'ec2InstanceType', 'subnetId', 'securityGroupId', 'iamRoleName'];
Object.assign(params, Object.fromEntries(Object.entries(config.input).filter(([k,v]) => passThrough.includes(k) && v)))

which is a bit dense

@jpalomaki
Copy link

@markpitchless Are you planning to post this version of launch template support (including this fix) back to upstream? If so, I'd be happy to close my draft PR in favor of that one.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants