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 configuration for controlling the autonaming behavior #1831

Merged
merged 5 commits into from
Nov 18, 2024

Conversation

corymhall
Copy link
Contributor

@corymhall corymhall commented Nov 14, 2024

This PR adds some new functionality to control the auto naming behavior.
The new behavior lives behind a provider config variable and must be
explicitly enabled by the user. The existing behavior will remain the
default behavior of the provider.

What's new

  • autoTrim: When this is set to true the provider will automatically
    trim the generated name to fit within the maxLength requirement.
  • randomSuffixMinLength: Set this to control the minimum length of the
    random suffix that is generated. This is especially useful in
    combination with autoTrim to ensure that you still end up with
    unique names (e.g. a random suffix of 1 character is not very unique)

closes #1816, re pulumi/pulumi-cdk#62

This PR adds some new functionality to control the auto naming behavior.
The new behavior lives behind a provider config variable and must be
explicitly enabled by the user. The existing behavior will remain the
default behavior of the provider.

**What's new**

- `autoTrim`: When this is set to true the provider will automatically
  trim the generated name to fit within the `maxLength` requirement.
- `randomSuffixMinLength`: Set this to control the minimum length of the
  random suffix that is generated. This is especially useful in
  combination with `autoTrim` to ensure that you still end up with
  unique names (e.g. a random suffix of 1 character is not very unique)

closes #1816, re pulumi/pulumi-cdk#62
@corymhall corymhall changed the title generate sdks Add configuration for controlling the autonaming behavior Nov 14, 2024
Copy link
Contributor

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

Attention: Patch coverage is 95.91837% with 2 lines in your changes missing coverage. Please review.

Project coverage is 49.96%. Comparing base (4111ee4) to head (7fad16d).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
provider/pkg/provider/provider.go 85.71% 0 Missing and 1 partial ⚠️
provider/pkg/resources/extension_resource.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1831      +/-   ##
==========================================
+ Coverage   49.62%   49.96%   +0.34%     
==========================================
  Files          43       43              
  Lines        6591     6636      +45     
==========================================
+ Hits         3271     3316      +45     
  Misses       3077     3077              
  Partials      243      243              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@corymhall corymhall self-assigned this Nov 15, 2024
if left <= 0 && autoTrim {
autoTrimMaxLength := autoNamingSpec.MaxLength - namingTrivia.Length() - randomSuffixMinLength
if autoTrimMaxLength <= 0 {
return resource.PropertyValue{}, fmt.Errorf("failed to auto-generate value for %[1]q."+
Copy link
Member

Choose a reason for hiding this comment

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

So surprised to see Fortran-style 1-based indices here but I think this checks out. This is right.

autoNameConfig: AutoNamingConfig{
AutoTrim: true,
},
comparison: within(12, 12),
Copy link
Member

Choose a reason for hiding this comment

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

Any chance we can see equals(resource.NewStringProperty("..")) here or it's omitted because it is random?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I might be able to do a regex check because only the end should be random.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@t0yv0
Copy link
Member

t0yv0 commented Nov 15, 2024

Added #1833 but out of scope of this PR.... Looks like we are consuming GetVariables still in this provider.

@t0yv0 t0yv0 self-requested a review November 15, 2024 23:22
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

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

LGTM 🚢

@t0yv0
Copy link
Member

t0yv0 commented Nov 15, 2024

Assuming here pulumi/pulumi#17592 is not quite ready for prime time so we need this now.

Copy link
Contributor

@flostadler flostadler left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -101,3 +135,25 @@ func getDefaultName(

return resource.NewStringProperty(random), nil
}

// trimName will trim the prefix to fit within the max length constraint.
// It will cut out part of the middle, keeping the beginning and the end of the string.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the right thing to do given the current constraints. But it could also be rather surprising to users.

For example, I often split my resource names with hyphens. This could generate some rather odd names.

But given that pulumi/pulumi#17592 is not ready yet, I think this is a good choice.
If we see the need for it, we could also expose more configuration options for autonaming in the future.

@@ -519,6 +520,14 @@ func (p *cfnProvider) Configure(ctx context.Context, req *pulumirpc.ConfigureReq
metadata.CfnCustomResourceToken: resources.NewCfnCustomResource(p.name, s3Client, lambdaClient),
}

if autoNaming, ok := vars["aws-native:config:autoNaming"]; ok {
var autoNamingConfig autonaming.AutoNamingConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test the unmarshalling? It seems to be untested right now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

Adding `startsWith` comparison to better assert autoTrim
@corymhall corymhall enabled auto-merge (squash) November 18, 2024 14:25
@corymhall corymhall merged commit 6f526ba into master Nov 18, 2024
18 checks passed
@corymhall corymhall deleted the corymhall/trim-autoname branch November 18, 2024 14:41
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.

Handle missing auto-naming constraints
3 participants