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

docs(appmesh): clarifying the difference between constructor and method to create a resource #15237

Merged
merged 16 commits into from
Jun 22, 2021

Conversation

Seiya6329
Copy link
Contributor

This closes #10049.

REV

  • Updating the doc for.add...() methods to specify that the resource is created in same stack where the Mesh exists
  • Updating README file to clarify the difference between using the constructor and method to create a resource.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@gitpod-io
Copy link

gitpod-io bot commented Jun 21, 2021

@mergify
Copy link
Contributor

mergify bot commented Jun 21, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@Seiya6329 Seiya6329 changed the title doc(appmesh): clarifying the difference between constructor and method to create a resource docs(appmesh): clarifying the difference between constructor and method to create a resource Jun 21, 2021
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

Looks good! Some small wording suggestions.

packages/@aws-cdk/aws-appmesh/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appmesh/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appmesh/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appmesh/lib/mesh.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appmesh/lib/mesh.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-appmesh/lib/mesh.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed skinny85’s stale review June 22, 2021 17:16

Pull request has been modified.

@Seiya6329
Copy link
Contributor Author

@skinny85 - Thanks for the corrections! They definitely look much better and easier to understand.

@Seiya6329 Seiya6329 requested a review from skinny85 June 22, 2021 17:18
skinny85
skinny85 previously approved these changes Jun 22, 2021
Copy link
Contributor

@skinny85 skinny85 left a comment

Choose a reason for hiding this comment

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

LGTM!

rrhodes and others added 10 commits June 22, 2021 11:59
Add support for Kinesis Streams in DynamoDB tables with a new `kinesisStreamArn` table option.

Closes aws#14534

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…part fails (aws#15166)

New-style ARNs are of the form `'arn:aws:s4:us-west-1:12345:/resource-type/resource-name'`.
We didn't handle that correctly in `parseArn()`, and instead returned an `undefined` resource,
which funnily enough should never happen according to our types.
Introduce the concept of ARN formats,
represented by an enum in core,
and replace the `Stack.parseArn()` method by a new one `Stack.splitArn()`,
taking that enum as a required second argument.

Spotted in https://github.com/aws/aws-cdk/pull/15140/files#r653112073

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
… its properties (aws#15208)

We currently do not check if inputs are token when validating them which might throw errors
if it is given a `ref`, adding them in here and corresponding unit tests that do not throw validation errors
when given tokens with invalid inputs.

Also updated one test which had mismatched description.

Also moved core to qualified import for better consistency. 

Testing done
------------------
* `yarn build && yarn lint & yarn test`


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…aws#15140)

Service Catalog AppRegistry application construct initial base version.
Please note the ARNS for this construct have two '/' in them hence the slightly different ARN parsing.

Testing done
------------------
* `yarn build && yarn test`
* `yarn integ`

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*



Co-authored-by: Dillon Ponzo <dponzo18@gmail.com>
The `rosetta/` directory contains fixtures that may be necessary to
compile code examples, and the `.lit.ts` represents module documentation
that may be necessary to generate the complete module documentation site
from the published artifacts. Those hence need to be included in the npm
package.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Removing the `BasicAuthConfig` property from the template doesn't remove
the basic auth. Explicitely set `EnableBasicAuth` to `false` instead.

Closes aws#15028


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
When asked to convert full hours to human strings, due to floating point errors, they get rendered as minutes. 1 hour gets converted to 60 minutes. 2 hours converts to 1 hour 60 minutes.

This request fixes that by first multiplying `amount` with the `fromUnit.inMillis` before dividing by `toUnit.inMillis`. This ensures that if the amount is big enough, it gets divided properly.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…hen endpoint is a token (aws#15219)

`Domain.fromDomainAttributes` throws the error "Invalid URL" when a token endpoint is provided. In this PR, the domain name is retrieved from the scope using the arn present in `DomainAttributes`, and if no name is found, we try to get it from the endpoint (as it has been done so far). Also added a test to verify that the error disappears with this change, even when it is given token attributes.

closes aws#15188

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@mergify mergify bot dismissed skinny85’s stale review June 22, 2021 19:08

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jun 22, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 884d499
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit d03c6cf into aws:master Jun 22, 2021
@mergify
Copy link
Contributor

mergify bot commented Jun 22, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

hollanddd pushed a commit to hollanddd/aws-cdk that referenced this pull request Aug 26, 2021
…od to create a resource (aws#15237)

This closes aws#10049.

#### REV
- Updating the doc for`.add...()` methods to specify that the resource is created in same stack where the `Mesh` exists
- Updating `README` file to clarify the difference between using the constructor and method to create a resource.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

[appmesh] VirtualNode in separate stack from Mesh creates circular dependency