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

cache: add cachecluster resource #189

Merged
merged 6 commits into from
Aug 7, 2020
Merged

cache: add cachecluster resource #189

merged 6 commits into from
Aug 7, 2020

Conversation

sahil-lakhwani
Copy link
Contributor

Signed-off-by: sahil-lakhwani sahilakhwani@gmail.com

Description of your changes

Adds CacheCluster resource

Example: examples/cache/memcached/cluster.yaml

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.

@negz negz closed this Jun 15, 2020
@negz negz reopened this Jun 15, 2020
@negz negz self-requested a review June 15, 2020 17:10
@negz negz self-assigned this Jun 15, 2020
@negz
Copy link
Member

negz commented Jul 1, 2020

@sahil-lakhwani Could you please move this PR out of draft, have someone from your team review it, and let us know when your internal review process has finished and you're ready for a Crossplane maintainer to review? Thanks!

@sahil-lakhwani
Copy link
Contributor Author

Hey @negz , in the planning meeting that of 06/15 - I think you had assigned it to yourself and agreed to review it hence I did not initiate separate review, but I will get it reviewed once internally now.

@ajaykangare
Copy link
Contributor

@sahil-lakhwani Added a few review comments, kindly address those and other looks good for me.

@ajaykangare ajaykangare self-requested a review July 8, 2020 06:54
rahulchheda
rahulchheda approved these changes Jul 8, 2020
View reviewed changes
Copy link
Contributor

@rahulchheda rahulchheda left a comment

Choose a reason for hiding this comment

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

Some small comments, code looks perfect

apis/cache/v1alpha1/cachecluster_types.go Outdated Show resolved Hide resolved
apis/cache/v1alpha1/cachecluster_types.go Show resolved Hide resolved
pkg/clients/elasticache/elasticache.go Outdated Show resolved Hide resolved
pkg/clients/elasticache/elasticache.go Outdated Show resolved Hide resolved
@negz negz marked this pull request as ready for review July 9, 2020 03:51
@sahil-lakhwani
Copy link
Contributor Author

@ajaykangare @rahulchheda I have addressed your comments, let me know if there are any more.

@negz I think this PR is ready for a maintainer to review.

hasheddan
hasheddan reviewed Jul 27, 2020
View reviewed changes
Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

This is looking pretty good @sahil-lakhwani :) Could you address some of the comments and also do a rebase on master since this has been open for some time?

apis/cache/v1alpha1/cachecluster_types.go Outdated Show resolved Hide resolved
apis/cache/v1alpha1/cache_subnet_group_types.go Outdated Show resolved Hide resolved
pkg/controller/cache/cluster/controller.go Outdated Show resolved Hide resolved
@sahil-lakhwani
cache: add cachecluster resource
ee31164 
Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>
@sahil-lakhwani
cachecluster: update provider reference
ac1b737 
Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>
@sahil-lakhwani
cachecluster: add test cases
4bb5093 
Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>
@sahil-lakhwani
update in code license
51972ee 
Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>
@sahil-lakhwani
cache: rectify license headers
8f6f025 
Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>
@sahil-lakhwani
Copy link
Contributor Author

@hasheddan I have addressed your comments.

@infinitecompute infinitecompute mentioned this pull request Aug 6, 2020
Closed
26 tasks
negz
negz approved these changes Aug 7, 2020
View reviewed changes
Copy link
Member

@negz negz left a comment

Choose a reason for hiding this comment

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

Thanks @sahil-lakhwani! A few nitpicks, but otherwise this looks great.

apis/cache/v1alpha1/references.go Outdated Show resolved Hide resolved
pkg/clients/elasticache/elasticache.go Outdated Show resolved Hide resolved
@negz
Apply suggestions from code review
b7b3dfb 
Signed-off-by: Nic Cope <negz@rk0n.org>
@negz negz merged commit be12f54 into crossplane-contrib:master Aug 7, 2020
wolffbe pushed a commit to wolffbe/provider-aws that referenced this pull request Feb 12, 2021
@sahil-lakhwani @negz
cache: add cachecluster resource (crossplane-contrib#189)
14c6637 
* cache: add cachecluster resource

Signed-off-by: sahil-lakhwani <sahilakhwani@gmail.com>

Co-authored-by: Nic Cope <nicc@rk0n.org>
namku pushed a commit to namku/provider-aws that referenced this pull request Mar 9, 2021
@sahil-lakhwani @negz
cache: add cachecluster resource (crossplane-contrib#189)
8abb80b 
* cache: add cachecluster resource


Co-authored-by: Nic Cope <nicc@rk0n.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hasheddan hasheddan hasheddan left review comments

@negz negz negz approved these changes

@rahulchheda rahulchheda rahulchheda approved these changes

@ajaykangare ajaykangare Awaiting requested review from ajaykangare

Assignees

@negz negz

Labels
ic new-resource
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sahil-lakhwani @negz @ajaykangare @hasheddan @rahulchheda @infinitecompute