-
Notifications
You must be signed in to change notification settings - Fork 392
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(storage): Add temporary execute method to storage init #990
Conversation
Manual checks:
outputs.yamlcoolBucket77BucketName:
Description: "The name of a user-defined bucket."
Value: !Ref coolBucket77
coolBucket77AccessPolicy:
Description: "The IAM::ManagedPolicy to attach to the task role"
Value: !Ref coolBucket77AccessPolicy% policy.yamlcoolBucket77AccessPolicy:
Type: AWS::IAM::ManagedPolicy
Properties:
Description: !Sub
- Grants CRUD access to the S3 bucket ${Bucket}
- { Bucket: !Ref coolBucket77 }
PolicyDocument:
Version: 2012-10-17
Statement:
- Sid: S3ObjectActions
Effect: Allow
Action:
- s3:GetObject
- s3:PutObject
- s3:PutObjectACL
- s3:PutObjectTagging
- s3:DeleteObject
- s3:RestoreObject
Resource: !Sub ${coolBucket77.Arn}/*
- Sid: S3ListAction
Effect: Allow
Action: s3:ListBucket
Resource: !Sub ${coolBucket77.Arn}% s3.yamlcoolBucket77:
Type: AWS::S3::Bucket
DeletionPolicy: Retain
Properties:
AccessControl: Private
BucketEncryption:
ServerSideEncryptionConfiguration:
- ServerSideEncryptionByDefault:
SSEAlgorithm: AES256
BucketName: !Sub '${App}-${Env}-${Name}-cool-Bucket77'
PublicAccessBlockConfiguration:
BlockPublicAcls: true
BlockPublicPolicy: true% params.yamlApp:
Type: String
Description: Your application's name.
Env:
Type: String
Description: The environment name your service, job, or workflow is being deployed to.
Name:
Type: String
Description: The name of the service, job, or workflow being deployed.% |
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.
Looks good to me, tiny nitpicks!
Just to clarify with everyone else, we're going to move forward with this temporary solution while the command is hidden so that folks can test it. Upcoming PRs will remove this change from Execute
and use the addons
and template
pkg.
internal/pkg/cli/storage_init.go
Outdated
ws: ws, | ||
addonWriter: ws, |
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.
Can we create a single interface that has both wsSvcReader
and wsAddonWriter
? this way we wouldn't need two fields.
Not the ideal name but maybe type wsAddonInit interface
internal/pkg/cli/storage_init.go
Outdated
switch o.StorageType { | ||
case dynamoDBStorageType: | ||
validator = dynamoTableNameValidation | ||
friendlyText = "DynamoDB table" |
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.
Do you think that for now we should update storageTypes
to exclude ddb? so that there is no confusion while using the command
internal/pkg/cli/storage_init.go
Outdated
Action: s3:ListBucket | ||
Resource: !Sub ${%[1]s.Arn}` | ||
cf := ` | ||
%[2]s: |
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.
huh i had no idea this was a thing, cool!
internal/pkg/cli/storage_init.go
Outdated
} | ||
|
||
// Strip non-alphanumeric characters from an input string | ||
func logicalIdSafe(input string) string { |
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 might be slightly simpler: https://golangcode.com/how-to-remove-all-non-alphanumerical-characters-from-a-string/
Golang best practices for logicalIDSafe and logicalIDName Move friendlyText string literal to const Trim trailing whitespace in CFN literals.
@@ -197,6 +205,93 @@ func (o *initStorageOpts) validateServiceName() error { | |||
return fmt.Errorf("service %s not found in the workspace", o.StorageSvc) | |||
} | |||
|
|||
func (o *initStorageOpts) Execute() error { | |||
params := ` |
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.
What do you think of moving those template to files in our templates
folder and modify the template
pkg so that in storage_init
we can just init a serializer struct and call Template()
to get the string? (Like what we did in svc_package
)
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 think we are just trying to have a test binary out by Monday, so this is a bandaid fix for now. We'll remove this code afterwards.
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.
+1 what Efe said. This is the fastest way to get an S3 bucket's CF output by the tool at the moment, and I will be adding something similar for DDB in the next day or so.
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.
Adds execute method which renders user inputs into cloudformation template in addons dir.
addresses #769
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.