-
Notifications
You must be signed in to change notification settings - Fork 4k
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 12 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,7 @@ export interface IClusterParameterGroup extends IResource { | |
* A new cluster or instance parameter group | ||
*/ | ||
abstract class ClusterParameterGroupBase extends Resource implements IClusterParameterGroup { | ||
|
||
/** | ||
* The name of the parameter group | ||
*/ | ||
|
@@ -62,17 +63,46 @@ export class ClusterParameterGroup extends ClusterParameterGroupBase { | |
*/ | ||
public readonly clusterParameterGroupName: string; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a parameter group name? |
||
|
||
/** | ||
* The parameters in the parameter group | ||
*/ | ||
readonly parameters: { [name: string]: string }; | ||
|
||
/** | ||
* The underlying CfnClusterParameterGroup | ||
*/ | ||
private readonly resource: CfnClusterParameterGroup; | ||
|
||
dontirun marked this conversation as resolved.
Show resolved
Hide resolved
|
||
constructor(scope: Construct, id: string, props: ClusterParameterGroupProps) { | ||
super(scope, id); | ||
|
||
const resource = new CfnClusterParameterGroup(this, 'Resource', { | ||
this.parameters = props.parameters; | ||
this.resource = new CfnClusterParameterGroup(this, 'Resource', { | ||
description: props.description || 'Cluster parameter group for family redshift-1.0', | ||
parameterGroupFamily: 'redshift-1.0', | ||
parameters: Object.entries(props.parameters).map(([name, value]) => { | ||
return { parameterName: name, parameterValue: value }; | ||
}), | ||
parameters: this.parseParameters(), | ||
}); | ||
|
||
this.clusterParameterGroupName = resource.ref; | ||
this.clusterParameterGroupName = this.resource.ref; | ||
} | ||
private parseParameters(): any { | ||
return Object.entries(this.parameters).map(([name, value]) => { | ||
return { parameterName: name, parameterValue: value }; | ||
}); | ||
} | ||
TheRealAmazonKendra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/** | ||
* Adds a parameter to the parameter group | ||
* | ||
* @param name the parameter name | ||
* @param value the parameter name | ||
*/ | ||
public addParameter(name: string, value: string): void { | ||
TheRealAmazonKendra marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const existingValue = Object.entries(this.parameters).find(([key, _]) => key === name)?.[1]; | ||
if (existingValue === undefined) { | ||
this.parameters[name] = value; | ||
this.resource.parameters = this.parseParameters(); | ||
} else if (existingValue !== value) { | ||
throw new Error(`The parameter group already contains the parameter "${name}", but with a different value (Given: ${value}, Existing: ${existingValue}).`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 throw new Error(`The parameter group id "${Names.uniqueResourceName(this, {})}" already contains the parameter "${name}", but with a different value (Given: ${value}, Existing: ${existingValue}).`); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. In this case it's set to the Instrinsic Considering that this is runtime error, I think using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Reverted back to this message in the latest commit |
||
} | ||
} | ||
} |
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.
This isn't rendering correctly. I suspect that the issue is the
fixture=cluster
piece causing it.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'll fix it for the example in this PR and raise an issue. Looking at the documentation it's also not rendering for the other examples using
fixture=cluster
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.
Actually I think none of the fixtures (even the default) are loading properly for the redshift alpha library
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.
#21907