Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(redshift-alpha): directly add parameters to a parameter group or indirectly through a cluster #20944
feat(redshift-alpha): directly add parameters to a parameter group or indirectly through a cluster #20944
Changes from 22 commits
a8aa2f8
8852e7b
05727b8
5c5f98b
f2d5015
1c8db26
fe97d47
c3e5e87
8ba1e4b
37b1c9a
1a7d327
271e3e2
e007770
0730a19
ad8140b
87bffb0
9c3b28b
220f871
2191f58
2a4fd32
b75471a
3621733
482e9ec
e36ab6b
fa4f365
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think now that you are using
this.cluster.clusterIdentifier
,Names.uniqueResourceId
is no longer needed in this context.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The clusterIdentifier can be undefined. It's being set to the optional clusterName prop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that happens before
this.clusterName
is set tothis.cluster.ref
.I think that I suggested using
Names.uniqueResourceName
instead ofNames.uniqueResourceId
somewhere in here, but since it wasn't used there, it doesn't make sense to use it here. It won't be named that and we'd be giving false information.You can set
up on line 528. That would work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with the false information.
It looks like the cluster Identifier needs to be unique account wide. So instead of setting the name to with
Names.uniqueResourceName
I think it might be better to fall back to the paramter group default description if the identifier isn't providedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the
Names.uniqueResourceName
and falling back to the defaultThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a parameter group name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add the name of the parameter group in here in case the user has an app with multiple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter groups don't have names, but I agree that it makes sense to identify it somehow , so maybe it makes sense to use
Names.uniqueResourceName
hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my most recent comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case it's set to the Instrinsic
Ref
ofCfnClusterParameterGroup
, so the error message would contain aToken
.Considering that this is runtime error, I think using
Names.uniqueResourceName
(along with the stack trace) should be helpful enough to locate the problematic parameter groupThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be providing the customer with inaccurate information. Sure, it's an approximation, but it's not actually the name of the resource. I suppose the stack trace will have to provide enough information in conjunction with the key value pair they're trying to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted back to this message in the latest commit