alexcasalboni / aws-lambda-power-tuning

AWS Lambda Power Tuning is an open-source tool that can help you visualize and fine-tune the memory/power configuration of Lambda functions. It runs in your own AWS account - powered by AWS Step Functions - and it supports three optimization strategies: cost, speed, and balanced.
Apache License 2.0
5.27k stars 362 forks source link

add ability to customize the prefix of the statemachine name #223

Closed TonySherman closed 5 months ago

TonySherman commented 6 months ago

This will address #220, allowing customized prefixes for the StateMachine name.

It is defaulted to the original name powerTuningStateMachine but the appended id will be slightly longer.

Current default: powerTuningStateMachine-8WZnCEWFsLBW

This PR changes the default to: powerTuningStateMachine-7c304ce0-a4ff-11ee-89b2-0ea1c2170c63

and allows the customized prefix: myCustomPrefix-7c304ce0-a4ff-11ee-89b2-0ea1c2170c63

TonySherman commented 6 months ago

Just drafting a PR to see what you think of the implementation. Have not addressed the Terraform changes yet.

TonySherman commented 6 months ago

I am not real familiar with Terraform but it looks like we could actually have a similar functionality by giving the statemachine a name_prefix instead of name

If that's the case, I think we just need to update this line

alexcasalboni commented 6 months ago

Thanks @TonySherman, this looks great 🚀

@ldcorentin any chance you can share your feedback on the Terraform side of things? 😄

alexcasalboni commented 6 months ago

@TonySherman I have added MaxLength to make sure the 44 chars limit is applied.

I would also play with AllowedPattern to make sure that we avoid restricted characters.

As for the documentation (here):

A name must not contain:
- white space
- brackets < > { } [ ]
- wildcard characters ? *
- special characters " # % \ ^ | ~ ` $ & , ; : /
- control characters (U+0000-001F, U+007F-009F)
TonySherman commented 6 months ago

@TonySherman I have added MaxLength to make sure the 44 chars limit is applied.

I would also play with AllowedPattern to make sure that we avoid restricted characters.

As for the documentation (here):

A name must not contain:
- white space
- brackets < > { } [ ]
- wildcard characters ? *
- special characters " # % \ ^ | ~ ` $ & , ; : /
- control characters (U+0000-001F, U+007F-009F)

I think the regex pattern I added should meet those requirements by limiting to alphanumeric characters.

alexcasalboni commented 5 months ago

@TonySherman I've done a few minor things:

It if all makes sense to you, I'll be happy to merge this PR :)

(don't worry about the failing integration test, it's an auth error on my side)

alexcasalboni commented 5 months ago

Also, I've realized we only have 43 characters for the prefix (not 44), since we're using one for the - delimiter :)

TonySherman commented 5 months ago

Good catch on the character count and the characters I missed! 😂

Those changes all look great to me! Really nice working with you on this!