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.

Sign up for GitHub

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

Jump to bottom

Bring S3 Bucket to v1beta1 #331

Merged
merged 11 commits into from
Oct 6, 2020
Merged

Bring S3 Bucket to v1beta1 #331

merged 11 commits into from
Oct 6, 2020

Conversation

krishchow
Copy link
Collaborator

@krishchow krishchow commented Sep 8, 2020 •
edited
Loading

Description of your changes

Fixes #99 #331
This PR aims to bring the S3 Bucket to v1beta1 by adding support for related resources and API calls for AWS S3.
Currently, there is support for the following API calls:

  • AccelerateConfiguration
  • Acl
  • CORS
  • Encryption
  • Inventory Configuration
  • Logging
  • Replication
  • RequestPayment
  • Tagging
  • Versioning
  • Website
  • NotificationConfiguration
  • LifecycleConfiguration

There are plans to implement support for:

  • AnalyticsConfiguration
  • InventoryConfiguration
  • MetricsConfiguration

Bucket Policy has already been implemented in a prior PR :)

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.

@krishchow krishchow requested a review from muvaf September 8, 2020 23:18
@krishchow
Copy link
Collaborator Author

Minor update, support for the following has been added:

  • LifecycleConfiguration
  • NotificationConfiguration

The following operations will need to be separated out into other resources, similar to the S3 Bucket Policy:

  • AnalyticsConfiguration
  • InventoryConfiguration
  • MetricsConfiguration

@muvaf
Copy link
Member

muvaf commented Sep 16, 2020

The following operations will need to be separated out into other resources, similar to the S3 Bucket Policy

@krishchow Do you plan on including those resources in this PR? I think deferring them to another PR would be better.

@krishchow
Copy link
Collaborator Author

@muvaf - That was my plan as well, I think they can be created a later date - as for this PR, the core logic for the controller/client should be finished now. I am just working on adding some testing for the new s3 bucket.

@krishchow krishchow marked this pull request as ready for review September 16, 2020 15:28
@krishchow krishchow force-pushed the S3BucketRefactor branch 2 times, most recently from 3bfd8df to f2e62b4 Compare September 16, 2020 20:53
@muvaf muvaf mentioned this pull request Sep 17, 2020
Closed
muvaf
muvaf reviewed Sep 18, 2020
View reviewed changes
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Great start! I have left some high level comments and went into a bit more detail for LifecycleConfiguration functionality, which could apply to other substructs. I believe, in general, we strive for the least amount of indirections and functions so that it's simpler to understand. That comes with some cost due to duplication or unaesthetic but the simplicity is usually worth it.

pkg/controller/s3/bucket.go Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/lifecycleConfig.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/lifecycleConfig.go Outdated Show resolved Hide resolved
apis/s3/v1beta1/bucket_types.go Outdated Show resolved Hide resolved
apis/s3/v1beta1/lifecycleConfiguration_types.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/lifecycleConfig.go Outdated Show resolved Hide resolved
@krishchow krishchow force-pushed the S3BucketRefactor branch 2 times, most recently from ad36109 to 1ddad67 Compare September 18, 2020 17:28
muvaf
muvaf reviewed Sep 22, 2020
View reviewed changes
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

It's coming together! I've left some comments around simplifications but I believe the approach is solid and makes it easier to deal with that archaic API.

pkg/clients/s3/bucketclients/notificationConfig.go Outdated Show resolved Hide resolved
apis/s3/v1beta1/bucket_types.go Show resolved Hide resolved
apis/s3/v1beta1/bucket_types.go Outdated Show resolved Hide resolved
apis/s3/v1beta1/lifecycleConfiguration_types.go Show resolved Hide resolved
apis/s3/v1beta1/loggingConfiguration_types.go Show resolved Hide resolved
pkg/clients/s3/bucket.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/clients.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/lifecycleConfig.go Outdated Show resolved Hide resolved
pkg/clients/s3/bucketclients/lifecycleConfig.go Outdated Show resolved Hide resolved
@muvaf muvaf mentioned this pull request Sep 24, 2020
Merged
4 tasks
@krishchow krishchow force-pushed the S3BucketRefactor branch 13 times, most recently from 545a389 to 4cffead Compare September 29, 2020 20:22
@krishchow
Copy link
Collaborator Author

@muvaf - any chance you can review this prior to Thursday? I believe pretty much everything should be done at this point aside from some minor cleanup work.

muvaf
muvaf approved these changes Sep 30, 2020
View reviewed changes
Copy link
Member

@muvaf muvaf left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. I have left some minor comments and I'd prefer not to use in.config at all in the resource controllers if possible since *v1beta1.Bucket is already available.

Let me know when this is ready to merge. I can follow up with a small PR for those comments.

.gitignore Outdated Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucket.go Show resolved Hide resolved
pkg/controller/s3/bucket.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
pkg/controller/s3/bucketresources/accelerateConfig.go Outdated Show resolved Hide resolved
@krishchow krishchow force-pushed the S3BucketRefactor branch 8 times, most recently from e226398 to f4f2648 Compare October 2, 2020 17:51
muvaf and others added 10 commits October 2, 2020 14:12
@muvaf @krishchow
s3 bucket: implemented all v1beta1 API structs, and removal of old S3…
529a65f 
… Bucket controller

Signed-off-by: Muvaffak Onus <onus.muvaffak@gmail.com>
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: finalized api definitions, began work on sub-controllers f…
18b7103 
…or creation, deletion and validation

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: added enum support, referencers for v1beta1 bucket
53a580d 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: refactored bucketcontrollers to add struct values, bucket,…
a598b64 
… expanded testing code for bucketcontrollers

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: renamed subcontroller functions observe, create and delete…
124095d 
… as requested in PR

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: late initialization added for buckets, renamed interface f…
d43de8d 
…unctions, moved to under pkg/controller

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: various fixes based on secondary review comments, began wo…
fe7d450 
…rk on subcontroller testing

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: renamed s3bucketpolicy client/controller to bucketpolicy
3a09188 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: finished subcontroller tests, reorganized bucket sub-contr…
e3222e7 
…oller testing, added all tests for the bucket resource

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: removed or regenerated rebase artifacts, removed redundent…
ec37068 
… bucket examples

Signed-off-by: Krish Chowdhary <krish@redhat.com>
@krishchow
s3 bucket: PR review changes
b935c3e 
Signed-off-by: Krish Chowdhary <krish@redhat.com>
@muvaf
Copy link
Member

muvaf commented Oct 6, 2020

Thank you @krishchow !! This is one of the oldest AWS API and we finally covered it with the managed reconciler!

@muvaf muvaf merged commit bac07bf into crossplane-contrib:master Oct 6, 2020
@muvaf muvaf mentioned this pull request Oct 6, 2020
Merged
2 tasks
@prasek prasek mentioned this pull request Oct 28, 2020
Closed
26 tasks
wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021
@muvaf
Merge pull request crossplane-contrib#331 from krishchow/S3BucketRefa…
0ce2c20 
…ctor

Bring S3 Bucket to v1beta1
namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021
@muvaf
Merge pull request crossplane-contrib#331 from krishchow/S3BucketRefa…
99d0d2a 
…ctor

Bring S3 Bucket to v1beta1
tektondeploy pushed a commit to gtn3010/provider-aws that referenced this pull request Mar 12, 2024
@MyzaTaras
Merge pull request crossplane-contrib#331 from MyzaTaras/issue-322
9ba7522 
Moving schemas (3) resources to v1beta1 version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muvaf muvaf muvaf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

S3 bucket API types and controllers to v1beta1 quality
2 participants
@krishchow @muvaf