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

Refacto aws_s3_bucket #471

Merged
merged 1 commit into from
May 6, 2021
Merged

Refacto aws_s3_bucket #471

merged 1 commit into from
May 6, 2021

Conversation

eliecharra
Copy link
Contributor

Q A
πŸ› Bug fix? no
πŸš€ New feature? no
⚠ Deprecations? no
❌ BC Break no
πŸ”— Related issues #347
❓ Documentation no

@eliecharra eliecharra added the kind/maintenance Refactoring or changes to the workspace label Apr 29, 2021
@eliecharra eliecharra requested a review from a team April 29, 2021 14:40
@codecov
Copy link

codecov bot commented Apr 29, 2021

Codecov Report

Merging #471 (98a43ac) into main (dc2b991) will increase coverage by 0.09%.
The diff coverage is 90.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #471      +/-   ##
==========================================
+ Coverage   69.61%   69.71%   +0.09%     
==========================================
  Files         286      286              
  Lines        6641     6653      +12     
==========================================
+ Hits         4623     4638      +15     
+ Misses       1648     1644       -4     
- Partials      370      371       +1     
Impacted Files Coverage Ξ”
pkg/resource/resource.go 64.84% <ΓΈ> (ΓΈ)
pkg/resource/aws/aws_s3_bucket.go 70.00% <57.14%> (-30.00%) ⬇️
pkg/middlewares/aws_bucket_policy_expander.go 86.95% <100.00%> (+6.00%) ⬆️
pkg/middlewares/s3_bucket_acl.go 100.00% <100.00%> (ΓΈ)
pkg/resource/aws/metadatas.go 100.00% <100.00%> (ΓΈ)
pkg/resource/schemas.go 72.97% <0.00%> (+10.81%) ⬆️

@eliecharra eliecharra force-pushed the refacto_aws_s3_bucket branch from d4d1613 to 14e2e5c Compare May 5, 2021 08:56
moadibfr
moadibfr previously approved these changes May 5, 2021
Copy link
Contributor

@moadibfr moadibfr left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/resource/aws/aws_s3_bucket_test.go Outdated Show resolved Hide resolved
pkg/middlewares/aws_bucket_policy_expander.go Outdated Show resolved Hide resolved
"policy": bucket.Policy,
"id": bucket.TerraformId(),
"bucket": bucketName,
"policy": policy,
Copy link
Contributor

Choose a reason for hiding this comment

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

We could totally write it that way since we won't need bucketName or policy var anymore when S3BucketPolicy will be refactored.

data := map[string]interface{}{
		"id":     bucket.TerraformId(),
		"bucket": (*bucket.Attrs)["bucket"],
		"policy": policyAttr,
	}

"permissions": []string{"FULL_CONTROL"},
"type": awssdk.String("CanonicalUser"),
"uri": awssdk.String(""),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

You can delete all the pointers for the attributes of each grant we won't need it anymore. Same for the other tests in this file.

wbeuil
wbeuil previously approved these changes May 6, 2021
@eliecharra eliecharra force-pushed the refacto_aws_s3_bucket branch from 43229f9 to 98a43ac Compare May 6, 2021 13:36
@eliecharra eliecharra merged commit 6386036 into main May 6, 2021
@eliecharra eliecharra deleted the refacto_aws_s3_bucket branch May 6, 2021 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/maintenance Refactoring or changes to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants