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

chore(CFN): check in CFN #1375

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

chore(CFN): check in CFN #1375

wants to merge 6 commits into from

Conversation

josecorella
Copy link
Contributor

Issue #, if available:

Description of changes:

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

Check any applicable:

  • Were any files moved? Moving files changes their URL, which breaks all hyperlinks to the files.

@josecorella josecorella requested a review from a team as a code owner March 12, 2024 18:43
lucasmcdonald3
lucasmcdonald3 previously approved these changes Mar 12, 2024
lucasmcdonald3
lucasmcdonald3 previously approved these changes Mar 13, 2024
lucasmcdonald3
lucasmcdonald3 previously approved these changes Mar 13, 2024
Comment on lines +1 to +6
Outputs:
StackArn:
Description: >-
Do not remove this output! Pipelines needs this to do its association. (And
LPT. Removing it will break things)
Value: !Ref 'AWS::StackId'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: If we maintain the CFN here, we cannot use BONES pipelines to maintain the Stack.

Suggested change
Outputs:
StackArn:
Description: >-
Do not remove this output! Pipelines needs this to do its association. (And
LPT. Removing it will break things)
Value: !Ref 'AWS::StackId'

Comment on lines +8 to +22
DeploymentBucketImportName:
Default: 'BONESBootstrap-PDX-beta-DeploymentBucket'
Description: >-
This parameter is meant to be passed by LPT (and piplines). It holds the
name of import that points to the bucket that holds your artifacts. You
should use this as the import (Fn::ImportValue: {Ref: DeploymentBucket})
for getting any BATS related artifacts.
Type: String
Stage:
Default: 'beta'
Type: String
PipelinesControlledRegionBucket:
Type: String
Description: The regionalized bucket to read the artifact from.
Default: 'placeholder'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: If we maintain the CFN here, we cannot use BONES pipelines to maintain the Stack.

Suggested change
DeploymentBucketImportName:
Default: 'BONESBootstrap-PDX-beta-DeploymentBucket'
Description: >-
This parameter is meant to be passed by LPT (and piplines). It holds the
name of import that points to the bucket that holds your artifacts. You
should use this as the import (Fn::ImportValue: {Ref: DeploymentBucket})
for getting any BATS related artifacts.
Type: String
Stage:
Default: 'beta'
Type: String
PipelinesControlledRegionBucket:
Type: String
Description: The regionalized bucket to read the artifact from.
Default: 'placeholder'

Comment on lines +68 to +70
ExampleWaitHandle:
Properties: {}
Type: 'AWS::CloudFormation::WaitConditionHandle'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: Remove Example code from yee old BONES template

Suggested change
ExampleWaitHandle:
Properties: {}
Type: 'AWS::CloudFormation::WaitConditionHandle'

Location: 'https://github.com/aws/aws-encryption-sdk-javascript'
ReportBuildStatus: 'true'
Type: GITHUB
Type: 'AWS::CodeBuild::Project'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Nit: I find it easier to read CFN if I know the Type first.

Suggested change
Type: 'AWS::CodeBuild::Project'

Resource:
- !Sub 'arn:aws:codebuild:${AWS::Region}:${AWS::AccountId}:project/JavaScriptESDK'
PolicyName: !Sub '${AWS::StackName}CloudWatchLogsPolicy'
Type: 'AWS::IAM::Role'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Nit: I find it easier to read CFN if I know the Type first.

Suggested change
Type: 'AWS::IAM::Role'

Comment on lines +31 to +32
CodeBuildRole:
Properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Nit: I find it easier to read CFN if I know the Type first.

Suggested change
CodeBuildRole:
Properties:
CodeBuildRole:
Type: 'AWS::IAM::Role'
Properties:

Comment on lines +71 to +72
JavaScriptESDK:
Properties:
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion/Nit: I find it easier to read CFN if I know the Type first.

Suggested change
JavaScriptESDK:
Properties:
JavaScriptESDK:
Type: 'AWS::CodeBuild::Project'
Properties:

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.

3 participants