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.

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

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

Merged

Conversation

TonySherman
Copy link
Contributor

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
Copy link
Contributor Author

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

@TonySherman
Copy link
Contributor Author

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
Copy link
Owner

Thanks @TonySherman, this looks great 🚀

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

@TonySherman TonySherman marked this pull request as ready for review January 4, 2024 16:26
@alexcasalboni
Copy link
Owner

@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
Copy link
Contributor Author

@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
Copy link
Owner

alexcasalboni commented Jan 15, 2024

@TonySherman I've done a few minor things:

  • update the regex to allow - and _ as well
  • update the SAR script sample file with the new param
  • update the doc files (also mentioning the allowed characters)
  • update the Terraform file to use name_prefix instead of name - I think this is a more useful solution that allows you to deploy the same stack to the same account twice without naming conflicts (there might be other conflicts though!)

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
Copy link
Owner

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

@TonySherman
Copy link
Contributor Author

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!

@alexcasalboni alexcasalboni merged commit 3af359f into alexcasalboni:master Jan 15, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants