-
Notifications
You must be signed in to change notification settings - Fork 83
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
RFC 499: AppConfig Constructs #500
Conversation
### Example | ||
```ts | ||
const appconfig = new AppConfig(this, 'MyAppConfig',{ | ||
deploymentStrategy: <IDeploymentStrategy>, |
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.
Other than AppConfig
, what construct is DeploymentStrategy
associated to, and how is it consumed?
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.
Other than AppConfig
, the DeploymentStrategy
will not be associated to anything else, however it could be potentially associated with a CfnDeployment
from our L1 constructs. The DeploymentStrategy
construct is only relevant when we are deploying configuration which the AppConfig
construct (and CfnDeployment
if users want) will handle. Does this answer your question of how is it consumed?
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.
It does. By the way, how does AWS::AppConfig::Deployment
work? Do you have to modify its logical ID every time you make a change to make sure your change is actually published (a la API Gateway)?
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.
You don't have to modify the logical ID, it looks like when I redeploy a AWS::AppConfig::Deployment
, it will create a new resource in CFN but it will still retain the old deployment
text/0499-appconfig-constructs.md
Outdated
deployTo: [ // will deploy to all environments by default | ||
Environment.create('beta'), | ||
Environment.create('prod'), | ||
Environment.from(<IEnvironment>), |
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.
What would this from
factory method do?
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 method will use the existing environment passed in instead of creating a new environment
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.
Why do you need a method at all in this case?
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.
Ah, you're right, I don't
text/0499-appconfig-constructs.md
Outdated
|
||
# AppConfig | ||
|
||
An AppConfig construct will be the simplest way to create and deploy configuration. This construct will handle deployments for you and start deploying configuration on creation. |
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.
Similar question to the configuration one: AppConfig
seems to be a higher-level version of Application
. Why have both?
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.
Same answer as above
text/0499-appconfig-constructs.md
Outdated
deployTo: [ // will deploy to all environments by default | ||
Environment.create('beta'), | ||
Environment.create('prod'), | ||
Environment.from(<IEnvironment>), |
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.
Why do you need a method at all in this case?
|
||
# Configuration | ||
|
||
A configuration is a higher level construct that can either be a HostedConfiguration (stored internally through AppConfig) or a SourcedConfiguration (stored in S3, Secrets Manager, SSM Parameter, SSM Document, or Code Pipeline). This construct will be used as a building block to deploy configuration through the AppConfig construct (detailed in the next section). For a HostedConfiguration, this construct will combine both a configuration profile and a hosted configuration version. |
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 see where you're coming from. But here's what I find confusing (and users also probably would): there is an L2 that is basically the same as the L1, and then there is an another L2, which looks very similar to the first one, but with additional features. Not only that, but the "higher-level" L2 can be constructed with a reference to the the "lower-level" L2.
Can you expand on the use case for importing existing resources?
### Example | ||
```ts | ||
const appconfig = new AppConfig(this, 'MyAppConfig',{ | ||
deploymentStrategy: <IDeploymentStrategy>, |
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.
It does. By the way, how does AWS::AppConfig::Deployment
work? Do you have to modify its logical ID every time you make a change to make sure your change is actually published (a la API Gateway)?
Responding to #500 (comment) since it wouldn't let me reply directly. I think this could be from a number of factors. For one, there could be a customer who has been deploying configuration through other means and want to migrate to managing config through CDK. Another possibility is people already using L1 constructs can easily migrate their resources to using L2 constructs. |
Can't you have the import methods in the higher-level constructs? |
text/0499-appconfig-constructs.md
Outdated
|
||
// optional | ||
name: 'ExtensionName', | ||
resources: [ |
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 will complicate the API unnecessarily. My suggestion:
- Keep
Extension
as simple as possible, serving only as an abstraction toCfnExtension
. In particular, don't add theresources
property to it. The list ofAction
s is a good addition, though. - Create a behavioral interface (e.g.,
IExtensible
) and makeAppConfig
,Environment
andIConfiguration
implement/extend it. This interface should have aon(ActionPoint, EventDestination)
. If you want, you can also add convenience methods such asonDeploymentComplete()
. When these methods are used, an extension association is created with the method's target. This is essentially what you are proposing down below forAppConfig
, but applying it to the other two types of resources as well. - Have an
addExtension
in this interface, so users can add extensions they created by instantiatingExtension
directly. - Maybe create a base class
ExtensibleBase
to avoid re-implementing the same thing multiple times.
text/0499-appconfig-constructs.md
Outdated
const extension = new Extension(this, 'MyExtension', { | ||
actions: [ | ||
new Action(this, 'MyAction', { | ||
actionPointEvent: <IFunction|IQueue|IBus|ITopic>, // ex. ActionPointEvent.fromFunction(<IFunction>) |
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 don't know if "action point event" is the term commonly used when talking about AppConfig (the data type in the CloudFormation spec is just Json
, so I can't tell what exactly an action entails). But, from my outside perspective, this name doesn't mean anything. I would expect it to be called something like eventDestination
or eventTarget
, with a matching data type name, like IEventDestination
or IEventTarget
.
By the way, here you can follow the s3.Bucket.addEventNotification()
pattern and create different implementations like LambdaDestination
, SqsDestination
etc.
This is a request for comments about AppConfig L2 constructs. See #499 for
additional details.
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache-2.0 license