-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
refactor(servicecatalogappregistry): prepare ApplicationAssociator for future extensions #22644
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.
Thank you for your efforts but this change does not align with our design principles and is a breaking change. Because this is an experimental module, breaking changes are allowed, but we have guidelines in our contributing guide for how to indicate them so that we will accept them.
Please review our contributing guide and general design guidelines. I won't just outright close this PR but if you plan to continue work on it, I'd like to understand what in the user experience you're trying to improve and why this is an improvement.
stackName: 'MyAssociatedApplicationStack', | ||
env: {account: '123456789012', region: 'us-east-1'}, | ||
}, | ||
appAssociatorProps: [appreg.ApplicationAssociatorPropsInputFactory.getApplicationAssociatorPropsFromAppName('MyAssociatedApplication', { |
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 line sets off a bit of red flags to me. Please talk in the customer's domain language, not in details of how the programming constructs work. Yes, I'm aware that in these case the customer is also a programmer, so there may be some who are not intimidated upon seeing the identifier ApplicationAssociatorPropsInputFactory
. There will also be some that are, who may think "this is too complicated for me", and we don't want to lose those people.
I usually phrase this as "name a thing after what it does, not what it is". So yes, while this class may be a "factory", the user doesn't give two licks whether it is a factory or not. They also wouldn't care if it was a "singleton", or if it was a "visitor". They only thing they care about is: "what class do I need to use to get instances of the given type (helpful to name the class closely after the prop name), and what happens when I use this class".
appAssociatorProps:
^^^^^ Why is this called "props" here?
What value does it add to a user to say these are props?
We are already IN a props object to an appAssociator. Why do
we have "appAssociatorProps" at a deeper level?
I think this is looking for a different term.
[appreg.ApplicationAssociatorPropsInputFactory.
^^^^^^^^^^^^^^^^^^^
Same here. Techincally speaking btw the
static method is the "factory", not the class
that holds it.
getApplicationAssociatorPropsFromAppName(
^^^^^ Don't start this with get. First of all, jsii won't let you,
and get is generally just a bad word to use for anything that's
not a property access in Java.
We can do shorter and sweeter:
const associatedApp = new appreg.ApplicationAssociator(app, 'AssociatedApplication', {
applications: [
appreg.Application.existingApplicationByArn('arn:aws:....:MyAssociatedApplication'),
appreg.Application.existingApplicationByName('myname', {
account: '1234', // Optional, defaults to current? Not sure if that's a good idea
region: 'us-east-1',
}),
appreg.Application.createApplicationStack({
stackName: 'MyAssociatedApplicationStack', // Optional
account: '123456789012', // Optional, defaults to whatever?
region: 'us-east-1'
}),
]
});
Now, obviously appreg.Application
already exists, so we need to find an alternative name for that. But keep it short and sweet and to the point and meaningful to the user please.
appAssociatorProps: [appreg.ApplicationAssociatorPropsInputFactory.getApplicationAssociatorPropsFromAppName('MyAssociatedApplication', { | ||
stackName: 'MyAssociatedApplicationStack', | ||
env: { account: '123456789012', region: 'us-east-1' }, | ||
}, 'Testing associated application')], |
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.
No more positional arguments after a "props" argument please. This is hard to read. Props arguments should be the last argument, anything else optional you want to add should just go into the props.
Pull request has been modified.
Each It would be useful to have The user can specify a single account for all AppRegistry application instances in the discovered Regions. What do you think? |
I agree with your suggestion but this PR is keep the construct future compatible while we evaluate the best way to incorporate multi region support |
stackProps: { | ||
stackName: 'MyAssociatedApplicationStack', | ||
}, | ||
applications: [appreg.ApplicationBuilder.importApplicationFromArn('arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId', { |
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.
We don't use import terminology for this anymore. "CDK Import" is overloaded enough as it is.
applications: [appreg.ApplicationBuilder.importApplicationFromArn('arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId', { | |
applications: [appreg.ApplicationBuilder.existingApplicationFromArn('arn:aws:servicecatalog:us-east-1:123456789012:/applications/applicationId', { |
stackName: 'MyAssociatedApplicationStack', | ||
env: {account: '123456789012', region: 'us-east-1'}, | ||
}, | ||
applications: [appreg.ApplicationBuilder.createApplication('MyAssociatedApplication', { |
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'm not sure ApplicationBuilder
covers the intent. Is this an instance of the Builder Pattern? No, so calling it a Builder
will raise incorrect expectations. All the more confusing given that in Java, we do generate builder patterns for CDK classes. So now you have builders, and builders, and they're not the same.
Some alternative names:
- ApplicationSource
- ApplicationStack
- TargetApplication
- AssociatableApplication
- AssociationTarget
- Association
- ...
* | ||
* @default - Empty array. | ||
*/ | ||
readonly applications: IBaseApplicationAssociatorProps[]; |
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.
readonly applications: IBaseApplicationAssociatorProps[]; | |
readonly applications: ITargetApplication[]; |
- Props never start with
I
BaseXxxProps
are calledXxxOptions
- These aren't props for the
ApplicationAssociator
, they relate totargetApplications
(or whatever)
/** | ||
* Class which constructs the input from provided Application ARN. | ||
*/ | ||
class ImportApplicationProps implements IBaseApplicationAssociatorProps { |
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.
Here's a pattern to follow for these classes, so you can put the behavior INSIDE the individual case classes (instead of in the main class which works by inspecting certain properties):
export abstract class ApplicationTarget {
public static existingApplication(...) {
return new ExistingApplicationTarget(...);
}
public static createApplication(...) {
return new CreateApplicationTarget(...);
}
public abstract bind(scope: Construct, options: BindApplicationTargetOptions): BindApplicationTargetResult;
}
export interface BindApplicationTargetOptions { /* if you need to pass options into the class */ }
export interface BindApplicationTargetResult {
readonly applicationArn: string;
/* any other output you need to make decisions on */
}
/* note: no export */ class CreateApplicationTarget extends ApplicationTarget {
constructor(...) {
super();
}
public bind(scope: Construct, options: BindApplicationTargetOptions): BindApplicationTargetResult {
const stack = new Stack(scope, '...', { ... });
new Application(stack, '...', { ... });
return {
applicationArn: 'arn:aws:...:application/MyName',
};
}
}
Put them in a separate file as well please.
@@ -56,17 +137,21 @@ export class ApplicationAssociator extends Construct { | |||
constructor(scope: cdk.App, id: string, props: ApplicationAssociatorProps) { | |||
super(scope, id); | |||
|
|||
const applicationStack = new cdk.Stack(scope, 'ApplicationAssociatorStack', props.stackProps); | |||
if (props.applications.length != 1) { | |||
throw new Error('Please provide either ARN or application 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.
This error message is not expressed in terms of API elements the user is familiar with, and is even inaccurate if length == 2
.
Please pass exactly 1 instance of ApplicationTarget.createApplication(), ApplicationTarget.existingApplicationFromArn(), etc, into the 'application' property
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…r future extensions (#22644) Updated Input Structure for Application Associator L2 Construct * This helps user to pass the input using factory pattern * Factory Pattern makes construct extensible ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* Co-authored by: Santanu Ghosh
Updated Input Structure for Application Associator L2 Construct
All Submissions:
Adding new Unconventional Dependencies:
New Features
yarn integ
to deploy the infrastructure and generate the snapshot (i.e.yarn integ
without--dry-run
)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Co-authored by: Santanu Ghosh