-
Notifications
You must be signed in to change notification settings - Fork 5
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: add parameter helper functions on GuStack #92
Conversation
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.
Left some comments in the diff
Definitely one for discussion. This is quite similar to something we had right at the start which we then moved away from. I like this general approach in theory but we didn't quite get it right the first time so resorted to just using |
I think I have a use case for this! 🎉 A
|
cba8f4a
to
05b9148
Compare
Another use case is then you want to have multiple ASGs in a stack. Currently an ASG will bring an AMI parameter with it. This parameter is called "AMI" by default. If we try to add two ASGs to a stack, then we'll attempt to add the same parameter twice and an exception is thrown. If we have a helper function to manage params, then we can do some validation. I'll revive this branch today. |
@akash1810 FYI I think it's possible to override this behaviour. |
Ah! I keep forgetting about this! I don't think this'll allow a single AMI param to be used in multiple ASGs however as a similar exception noted above will be thrown? Sharing AMIs across ASGs is fairly common and reasonable; here's an example from Grid the single param |
I haven't tested this, but I think this might work?
|
05b9148
to
5d98879
Compare
Oh lovely! Yeah that does work! The tests fail due to |
c434732
to
c1f0888
Compare
Helper functions to get/set parameters on the GuStack. An `Error` is thrown when getting a parameter that hasn't been set; this is to catch the error at compile time.
A GuParameter now relies on a function from GuStack, as GuStageParameter and GuStackParameter are automatically added to a GuStack, we cannot explicitly test them now as its recursuve. Remove them in favour of a slightly implicit test in stack.test.ts.
These can be replaced with the new getParam helper.
c1f0888
to
40292cb
Compare
Merging w/out full approval as @jorgeazevedo is currently on leave. |
What does this change?
#65, but for parameters.
Helper functions to get/set parameters on the GuStack. An
Error
is thrown when getting a parameter that hasn't been set; this is to catch the error at compile time.This is useful for when a construct reads from a parameter but isn't responsible for adding the parameter. For example, reading the
DistributionBucketName
param in the user data, whereDistributionBucketName
is added byGuGetDistributablePolicy
viaGuInstanceRole
. One such example can be seen here.Does this change require changes to existing projects or CDK CLI?
No.
How to test
Observe CI.
How can we measure success?
It is simpler to share parameters across constructs.
Have we considered potential risks?
n/a