Skip to content
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(amplify): Add resource support (wip) #3209

Closed
wants to merge 65 commits into from
Closed

feat(amplify): Add resource support (wip) #3209

wants to merge 65 commits into from

Conversation

sthulb
Copy link
Contributor

@sthulb sthulb commented Jul 5, 2019

Start of the Amplify CDK L2 construct

fixes issue: #3207

@sthulb sthulb requested a review from a team as a code owner July 5, 2019 08:36
@sthulb sthulb changed the title feat: Amplify Console wip: feat: Amplify Console Jul 5, 2019
@sthulb sthulb changed the title wip: feat: Amplify Console feat(amplify): Add resource support (wip) Jul 10, 2019
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/branch.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/package.json Show resolved Hide resolved
@sthulb sthulb requested a review from a team as a code owner July 21, 2019 09:12
@sthulb
Copy link
Contributor Author

sthulb commented Jul 21, 2019

@eladb I think this is about done – mind reviewing it again?

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed only app.ts but I believe many comments apply across the board. Let's do another iteration after you apply them throughout...

Also:

  • Missing README
  • All inline documentation is lacking. Preferably, copy & paste from the official AWS documentation with as much details as possible. Your goal is for people not to need to leave the IDE

packages/@aws-cdk/aws-amplify/package.json Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
packages/@aws-cdk/aws-amplify/lib/app.ts Show resolved Hide resolved
@jogold
Copy link
Contributor

jogold commented Jul 29, 2019

Hey @sthulb, it would be nice to support the property AutoBranchCreation (Use the AutoBranchCreationConfig property type to automatically create branches that match a certain pattern., https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-amplify-app.html#cfn-amplify-app-autobranchcreationconfig) from the start.

(needs an update of the CFN specs).

AWS Amplify Console adds support for automatically deploying branches that match a specific pattern

@sthulb
Copy link
Contributor Author

sthulb commented Jul 29, 2019

@jogold Indeed – I plan on supporting it in another PR. I want to get this one merged and then I'll start on the new features

Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inline docs are still lacking. Make sure all public APIs (incl. arguments) are fully documented.

This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project.
Define a new App:
```ts
new App(this, 'MyApp', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use amplify.App. This is the idiomatic way we reference resources within construct library modules and also we want to disambiguate with core.App, which we idiomatically refer to as App.

@@ -15,8 +15,79 @@
---
<!--END STABILITY BANNER-->

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide some background about what is the Amplify service and what is this library capable of doing. As much as possible copy details and links from AWS documentation.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Amplify service (known as the Amplify Console) offers continuous deployment and fully managed hosting for fullstack serverless web apps.

@@ -15,8 +15,79 @@
---
<!--END STABILITY BANNER-->

This module is part of the [AWS Cloud Development Kit](https://github.com/aws/aws-cdk) project.
Define a new App:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain what an amplify app is. assume throughout this doc that people don't need to open 100 browser tabs to understand what they need to do. This should be as self-contained as possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An Amplify App is a collection of branches that represent frontend and backend environments.

@jogold
Copy link
Contributor

jogold commented Aug 1, 2019

@jogold Indeed – I plan on supporting it in another PR. I want to get this one merged and then I'll start on the new features

#3504

@jogold
Copy link
Contributor

jogold commented Aug 21, 2019

@sthulb is this still under development?

@sthulb
Copy link
Contributor Author

sthulb commented Aug 21, 2019

Yep. I’m on holiday at the moment.

@NGL321 NGL321 self-assigned this Oct 10, 2019
@jogold
Copy link
Contributor

jogold commented Oct 23, 2019

@sthulb is this still under development?

@sthulb
Copy link
Contributor Author

sthulb commented Oct 23, 2019

sort of, it's on my to-do list of things.

@mergify
Copy link
Contributor

mergify bot commented Nov 1, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist
:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@sthulb sthulb closed this Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants