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

Update docs #2

Merged
merged 1 commit into from May 31, 2018
Merged

Update docs #2

merged 1 commit into from May 31, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented May 30, 2018

  • Changed wording, add more references between sections.
  • Highlighted some details better.
  • Reorganized discussion about L1s and L2s a bit to make it flow better.
  • Changes the title of the section that used to be called "using l2s"
    into "writing CDK apps".
  • Update formatting.
  • Get rid of level1, level2 substitutions

@rix0rrr rix0rrr changed the title Huijbers/docs update Update docs May 30, 2018
@@ -22,6 +22,10 @@
#
# AWS Sphinx configuration file.
#
# For more information about how to configure this file, see:
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert


.. _multiple_environments:

Creating Multiple Environments
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this under cloudformation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Don't even know where this came from, this is confusing because it's not even implemented yet.

because in most cases the |cdk| initializes new
constructs from within the context of their parent construct,
so |this| represents the parent.
In almost call cases, you will want to pass the keyword ``this`` for the ``parent``
Copy link
Contributor

Choose a reason for hiding this comment

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

"call" => "all"

@@ -50,26 +50,32 @@ to instantiate the ``StorageLayer`` construct.

When you initialize a construct,
add the construct to the construct tree by specifying the parent construct as the first initializer parameter,
a unique (to your stack) identifier for the construct as the second parameter,
and an optional set of properties for the final parameter,
a unique identifier for the construct as the second parameter,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should mention that it needs to be unique amongst siblings and not globally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed about removing the globally part as it's wrong.

Don't necessarily agree on adding the qualifier on the uniqueness here. Reasons:

  • It makes the sentence longer and harder to read
  • The constraint is quite logical and can go without saying. This is the same as declaring variables, basically, which people are already used to.
  • The constraint is also mentioned below under "construct names", which will be the more canonical location to put all reference information on construct names.
  • The error message you get when you do it wrong is very immediate and descriptive.

All in all, very low probability of a user doing it wrong and the impact is negligible, so no need for handholding on this I think.

Using |level1| Constructs
###################

|level1| constructs are all constructs found in the :py:mod:`aws-cdk-resources`
Copy link
Contributor

Choose a reason for hiding this comment

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

Sentence feels convoluted "level1 constructs are all constructs found...."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Are those constructs found..." ? Or just "are found..." ?


In general, you shouldn't need to use this type of Constructs, unless you have
special requirements or there is no |level2| package for the AWS resource you
need yet. Prefer using other packages with higher-level constructs instead. See
Copy link
Contributor

Choose a reason for hiding this comment

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

"Prefer to use the AWS Construct Library"

I would recommend to stop use the replacements |level1| and |level2| because it will make reviewing these docs much easer. I think settled on: AWS Construct Library (or just AWS Constructs) (for L2) and CloudFormation Resource Constructs (or just CloudFormation Resources) for (L1s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually completely agree. It also allows us to leave off the constant "AWS" qualifier, which is technically correct but makes every usage of the term horribly long and convoluted to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh by the way, that IS different terminology than we were using thus far. We were using

  • AWS Resource Construct
  • AWS Construct Library Construct

I do think I like yours better, so I'll do a replace on them.

- Changed wording, add more references between sections.
- Highlighted some details better.
- Reorganized discussion about L1s and L2s a bit to make it flow better.
- Changes the title of the section that used to be called "using l2s"
  into "writing CDK apps".
- Update formatting.
- Get rid of level1, level2 substitutions
@rix0rrr
Copy link
Contributor Author

rix0rrr commented May 31, 2018

I force-pushed a squash, so it looks like nothing happened but actually the diff got updated.

@rix0rrr rix0rrr merged commit a2f7991 into master May 31, 2018
@rix0rrr rix0rrr deleted the huijbers/docs-update branch May 31, 2018 17:58
mpiroc added a commit that referenced this pull request Jul 11, 2018
john-shaskin added a commit to john-shaskin/aws-cdk that referenced this pull request Jan 18, 2019
# This is the 1st commit message:

Add MethodResponse support for aws-apigateway

# This is the commit message aws#2:

Remove Dockerfile that was no longer needed

# This is the commit message aws#3:

Update python base image from 3.6 to 3.6.5

# This is the commit message aws#4:

Make the dockerfile work
john-shaskin added a commit to john-shaskin/aws-cdk that referenced this pull request Jan 18, 2019
# This is the 1st commit message:

Add MethodResponse support for aws-apigateway

# This is the commit message aws#2:

Remove Dockerfile that was no longer needed

# This is the commit message aws#3:

Update python base image from 3.6 to 3.6.5

# This is the commit message aws#4:

Make the dockerfile work
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
@fogfish fogfish mentioned this pull request Oct 21, 2019
2 tasks
mergify bot pushed a commit that referenced this pull request Jun 2, 2020
Attempt #2 
our diff command represents the URLSuffix (typically amazonaws.com)
by using the CloudFormation pseudo-parameter.

modifies the principal in the test to expect it instead. ran tests locally.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
smguggen referenced this pull request in smguggen/aws-cdk Aug 24, 2021
# This is the 1st commit message:

Add Identity Pool construct

# This is the commit message #2:

Bug fixes

# This is the commit message #3:

Bug fixes

# This is the commit message #4:

Formatting

# This is the commit message #5:

Add construct methods

# This is the commit message #6:

Remove flat

# This is the commit message #7:

Fix issues
@Crycham Crycham mentioned this pull request Aug 5, 2022
2 tasks
mrgrain added a commit that referenced this pull request Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants